-
Notifications
You must be signed in to change notification settings - Fork 36
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
DOP-5047: Font face of banners are 16px at load then change to 13px #1272
Conversation
✅ Deploy Preview for mongodb-snooty ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -19,7 +19,7 @@ export const baseBannerStyle = css` | |||
} | |||
|
|||
/* Force all content to be 13px in banners */ | |||
font-size: ${theme.fontSize.small}; | |||
font-size: ${theme.fontSize.small} !important; |
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.
instead of having an !important
(this will be harder to overwrite later), can we address this in the banner component where we are using a mix of styled
and css
.
The preference is to use import { css, cx } from @leafygreen-ui/emotion
since this is the same package they will be using. And for applying classes, we should be using the className prop with a cx(css"display:block;")
kind of style. This kicks in during HTML build (versus during hydration currently) and can be verified with a build version and looking at the raw HTML file
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.
Hi Seung, I was having issues implementing your suggested fix so I did something a little different. I removed the !important
and added the baseFontSize
props to the Leafygreen Banner component. If this fix doesn't suffice please let me know and I can do more research into getting the "display:block" to work.
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.
sorry, that was a bad example. the display block was irrelevant, its the usage of leafygreen/emotion that is key (versus current @emotion/styled)
A good example can be seen here in Card components, where we are using the className property of the LG component, and adding a cx(css"")
style string.
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.
changes LGTM. but the package diff should not be there. suggest merging main branch into yours, and running npm ci --legacy-peer-deps
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.
LGTM !
Stories/Links:
DOP-5047:
Current Behavior:
current behaviuor
Staging Links:
staged behavior
Notes:
Link to code ^
README updates