Image placeholder amp support #4334
Conversation
…placeholder, added switching logic for dark mode
|
Checkout your storybook preview here http://psammead-preview.tools.bbc.co.uk/4334 |
| const bgImageDark = `data:image/svg+xml;base64,${BBC_BLOCKS_DARK_MODE}`; | ||
| const bgImageRegular = `data:image/svg+xml;base64,${BBC_BLOCKS}`; | ||
|
|
||
| const AmpImgPlaceholderContainer = ({ |
There was a problem hiding this comment.
Attempted to use emotion to create a styled component instead of using the inline style method. However currently fallback isn't supported in the installed version of emotion as a valid prop so it is being stripped out on render. This has been fixed here: emotion-js/emotion#2224 and is now supported in the latest version of emotion.
A seperate issue will be created to amend this once all the emotion issues are fixed within Simorgh and Psammead is updated to the latest version of emotion.
This is what it should look like:
const AmpImgPlaceholderContainer = styled.div`
background-color: ${({ darkMode }) => (darkMode ? C_SHADOW : C_LUNAR)};
`;
There was a problem hiding this comment.
We have upgraded to Emotion 11, have we not got the version with this fix?
There was a problem hiding this comment.
We're currently on version 11.0.0 https://github.com/bbc/psammead/blob/4737f88e2cf5efab353b32c916817c2615d23c23/package.json#L46-L48
There was a new version 11.1.5 that came out a few days ago which included the fix from emotion-js/emotion#2224.
But it looks like it's been reverted https://www.npmjs.com/package/emotion, probably due to the Opera mini mishap. So currently there isn't a live build with the fix. Although I tested it before it was reverted and it worked a treat👌
There was a problem hiding this comment.
We've put it back to a pretty new version, I think we might wanna upgrade and use the new feature.
There was a problem hiding this comment.
Yep definitely agree, going to merge this PR in first and apply the improvements in a follow up PR as this has been ongoing for quite some time now.
| ); | ||
| }; | ||
|
|
||
| const AmpImgPlaceholder = props => ( |
There was a problem hiding this comment.
Attempted to also use emotion here but for some reason the styling wasn't applying and seemed to be stripped out. Here's the code I tried:
const AmpImgPlaceholder = styled('amp-img')`
position: 'absolute';
top: '50%';
left: '50%';
transform: 'translate(-50%, -50%)';
`;There was a problem hiding this comment.
Hmm, maybe worth raising an issue in the emotion repo, there seems to be patchy support for styling amp components?
| @@ -0,0 +1,91 @@ | |||
| /* eslint react/prop-types: 0 */ | |||
There was a problem hiding this comment.
See #4334 (comment)
HarveyPeachey
added
cross-team
| const bgImageDark = `data:image/svg+xml;base64,${BBC_BLOCKS_DARK_MODE}`; | ||
| const bgImageRegular = `data:image/svg+xml;base64,${BBC_BLOCKS}`; | ||
|
|
||
| const AmpImgPlaceholderContainer = ({ |
There was a problem hiding this comment.
We have upgraded to Emotion 11, have we not got the version with this fix?
| ); | ||
| }; | ||
|
|
||
| const AmpImgPlaceholder = props => ( |
There was a problem hiding this comment.
Hmm, maybe worth raising an issue in the emotion repo, there seems to be patchy support for styling amp components?
Resolves bbc/simorgh#8811
Overall change: Adds an amp-image fallback component for amp pages. This shows a placeholder while the amp-image is loading and also a fallback if the image cannot load. All the sizes and styling for the placeholder and fallbacks are based on the existing canonical fallback whilst adhering to AMP's fundamentals.
Code changes:
Images