-
Notifications
You must be signed in to change notification settings - Fork 14
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
docs(PPDSC-1976): Media list fix #179
Conversation
You can preview these changes on: |
Thanks @tbradbury for reviewing the code 👍 |
@@ -43,6 +44,9 @@ export const StyledCardContainerMedia = styled.div< | |||
box-sizing: border-box; | |||
display: block; | |||
position: relative; | |||
svg { | |||
${getBorderCssFromTheme('borderRadius', 'borderRadiusRounded010')}; | |||
} |
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.
When should we apply stying directly like this instead of in the style presets for the component?
I'm still trying to wrap my head around the various ways we handle styling!
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.
🤔 I guess we do not support svg
inside style presets atm?
also not sure if we could add it as nested object in the base? @pdimova
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.
@mstuartf and @Vanals Thank you for your time for giving valuable feedback.
This style changes has to be updated nested SVG. I was also initialy thinking the same, but after reffered some of the component styles files doubt is cleared and for the consistency updated the same way which those component follows. For example Toast and Flag component has the same nested structure in their styles files.
But, still please suggest me if we need to do any changes?
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.
👍 If this is a pattern that we follow elsewhere then it's fine for me. But I would be interested to understand why it doesn't work using defaults / style presets.
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.
Ok @mstuartf, thanks.
* docs(PPDSC-1976): Media List UI Fix
PPDSC-1976
What
I have done:
I have tested manually:
Before:
After:
Who should review this PR:
How to test: