Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Render amp-img for AMP HTML version #681

Closed
sareh opened this issue Sep 20, 2018 · 6 comments
Closed

Render amp-img for AMP HTML version #681

sareh opened this issue Sep 20, 2018 · 6 comments
Labels
AMP Work related to AMP blocked This issue should not be worked on until another internal issue is completed - see desc for details

Comments

@sareh
Copy link
Contributor

sareh commented Sep 20, 2018

Is your feature request related to a problem? Please describe.

We currently render <img> elements for the AMP HTML pages. We should render <amp-img> elements instead.

Example URLs:
http://localhost:7080/news/articles/amp/c0000000027o AMP
http://localhost:7080/news/articles/c0000000027o Canonical

Describe the solution you'd like
We should use the amp prop in the Article Container. We could pass down the value as a prop, or we could use Context API.

The current path our application logic takes is through these components:

/containers/Article
/containers/MainContent
/containers/Image
/containers/Figure
/components/Figure/Image

[*corrected as per @pjlee11 's comment below]
The Image component file renders an <img> element.

We should discuss whether it is worth using the Context API before a PR is opened for this work.
We should discuss whether we want an component/AmpImage or a parent component/Image that exports both an AmpImg and a Img.

The choices made here will form the pattern for conditional rendering of alternate components, so it is worth discussing in depth.
We should particularly think about the re-usability of components and containers, when they are pulled out into a separate component library.

Testing

  • Cypress test
  • npm run test:ampValidate should have no errors related to amp-image
@sareh sareh added this to the 05. News Image milestone Sep 20, 2018
@chris-hinds
Copy link
Contributor

At first the context API seems overkill for this. But thinking about it, I would prefer a component that may need to change based on the AMP prop to simply be wrapped in a HOC that consumes the context. This way we do not have to worry about passing the prop down to each component that would be dependent on it and adding more values to the context in the future would mean they are already available to each component.

With the Image component I would have thought a Parent image component would have been better as this could handle any particular markup etc and simply pass the standard image or the amp-image as it's children. That way markup and styles are consistent between the too. We are simply swapping out the actual tag.

@pjlee11
Copy link

pjlee11 commented Sep 20, 2018

The final component in the description /component/Image should actually be /component/Figure/Image. This therefore makes it clearer that we could create a new component, /component/Figure/AmpImage, and this could be conditionally injected into the the Figure inside of the FigureContainer.

Currently the ImageContainer acts as a data container extracting the necessary props from the data feed and passing it down to the FigureContainer. The FigureContainer then acts as a rendering container, currently handling the optional rendering of the copyright and caption, and this is where I would suggest the logic to switch between and .

My suggestion would be that ImageContainer should use the Context API to extract the amp value and then pass this into the FigureContainer. The the FigureContainer can conditionally render either /component/Figure/Image or /component/Figure/AmpImage.

@sareh sareh added Refinement Needed This is ready for refinement. It shouldn't be worked on until it has been refined by Dev & Test. AMP Work related to AMP labels Sep 20, 2018
@pjlee11
Copy link

pjlee11 commented Sep 20, 2018

To save having a seperate context API for just amp we should pass the amp prop down into the image container and then into the figure container

@jtart jtart added blocked This issue should not be worked on until another internal issue is completed - see desc for details and removed Refinement Needed This is ready for refinement. It shouldn't be worked on until it has been refined by Dev & Test. labels Sep 20, 2018
@jtart
Copy link
Contributor

jtart commented Sep 20, 2018

Blocked by #678.

@BogdanDogaru BogdanDogaru removed the blocked This issue should not be worked on until another internal issue is completed - see desc for details label Oct 17, 2018
@BogdanDogaru
Copy link
Contributor

Blocked pending resolving a possible bug in our AMP route (on our composition layer)

@jtart jtart added the blocked This issue should not be worked on until another internal issue is completed - see desc for details label Oct 17, 2018
@ChrisBAshton
Copy link
Contributor

Superseded by #953.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMP Work related to AMP blocked This issue should not be worked on until another internal issue is completed - see desc for details
Projects
None yet
Development

No branches or pull requests

6 participants