-
Notifications
You must be signed in to change notification settings - Fork 844
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
[CSS-in-JS] Create font scale functions/mixins #5822
Conversation
- Moved typography values into Amsterdam theme - Added `lineHeightMultiplier` back into theme - Simplified typography functions and moved to functions folder
Preview documentation changes for this PR: https://eui.elastic.co/pr_5822/ |
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.
Ready for review! 😄
useIsWithinBreakpoints(['xs', 's', 'm']) && mobileOptions.width | ||
useIsWithinBreakpoints(['xs', 's']) && mobileOptions.width |
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 noticed that the JS breakpoint styles were not in sync with the Sass breakpoint styles for mobile table layouts. This fixes that
// onUpdate={(value) => updateScale(key, value)} | ||
// numberProps={{ step: 0.1, style: { width: '6em' } }} | ||
groupProps={{ alignItems: 'center' }} |
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'd love some help on seeing if we can have these body values updatable. The difference here, that I couldn't quite work out, is that the options from a specific list of of keys available in the theme. So we'd need a new form control type (EuiSelect) to handle that.
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.
Is this something you want to address in this PR? I'm not sure I understand the problem well enough to help at the moment.
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.
Not necessarily, if it seems too complicated. Probably best to get the hooks in now.
Preview documentation changes for this PR: https://eui.elastic.co/pr_5822/ |
Font sizing is provided through this React hook (or function | ||
version) and not the global theme. It returns both the{' '} | ||
<EuiCode>font-size</EuiCode> and <EuiCode>line-height</EuiCode> for | ||
the provided <EuiCode>scale</EuiCode>. |
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 ❤️ this. Super smart way to do the theming "mixins" to also be used as "css utilities". Makes things very flexible.
Setting a precedent to us `_` when declaring types
Preview documentation changes for this PR: https://eui.elastic.co/pr_5822/ |
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!
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.
This looks great @cchaos! 🎉
Tested locally using the tokens and the useEuiFontSize
hook. I also customized the theme by changing the typography settings. Everything worked well! 😄
Just found one minor issue in one snippet.
Co-authored-by: Elizabet Oliveira <[email protected]>
Preview documentation changes for this PR: https://eui.elastic.co/pr_5822/ |
[NEW]
euiFontSize()
anduseEuiFontSize()
Documentation link: Building
Pixel value comparison to Sass version:
Also:
SCALES
toFONT_SCALES
Checklist
[ ] Checked in mobile[ ] Checked Code Sandbox works for any docs examples[ ] Checked for breaking changes and labeled appropriately[ ] Checked for accessibility including keyboard-only and screenreader modes[ ] Updated the Figma library counterpart