-
Notifications
You must be signed in to change notification settings - Fork 4
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
Added responsive styles to embeds #172
Conversation
lilia1891
commented
Sep 27, 2023
- Individual embeds: added responsive styles to iframes according to the received CSS classes;
- RichText embeds: added styles for the received responsive object class.
One minor naming suggestion. Otherwise, looks good, but maybe @terotik should take a look at the actual CSS. |
components/common/RichText.tsx
Outdated
@@ -52,6 +52,22 @@ type RichTextImageProps = { | |||
}; | |||
}; | |||
|
|||
const RichTextStyles = styled.div` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because these styled components are still components, I wouldn't name it as just --Styles, even thought styling is here the only reason for the component's existence. I would maybe call these StyledRichText or RichTextContainer or something like that. (I think we have used at least Styled* elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed. Thank you for a suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me thanks!
aspect-ratio
css is fairly new (to me at least) so I'm slightly concerned about the browser support. Caniuse gives 89.98% global support.
It's mainly the slightly older versions and IE who lack support. We could consider this fallback if we get reports of embeds not working in some browsers.
Ok to relase from me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good! There was another case with the same naming thing, otherwise great