-
Notifications
You must be signed in to change notification settings - Fork 863
[Emotion] Convert EuiTitle #5842
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
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5842/ |
cchaos
left a comment
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.
Update documentation around the Sass mixin or variable
I highly suggest adding documentation around the new JS version of euiTextBreakWord. You can just follow the same pattern as we did for Scroll and create a new Utility page for Text and move the CSS utilities there too. If you really want to do that in a follow up, that's fine as long as we're tracking those somewhere. But I'm hoping not to fall into the trap where we had so many undocumented but helpful Sass helpers/mixins.
- not yet touching the mixin
- and per-theme titles settings - just amsterdam for now, but we can possibly expand this if we add more themes in the future + delete old global_styling title files in favor of component-specific logic
+ fix test failure due to emotion sometimes generating a random 6 character className
e828c51 to
dfc1e0a
Compare
|
I think that should be all your comments addressed @cchaos (excepting |
dfc1e0a to
c5ebeff
Compare
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5842/ |
cchaos
left a comment
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.
Just a couple code nits while I spot check some of these downstream components
|
|
||
| export interface _EuiThemeTitle { | ||
| /** | ||
| * A font weight key for setting the base title weight |
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.
| * A font weight key for setting the base title weight | |
| * A font weight key for setting the base weight for titles and headings |
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.
We'll also need to add this to the Customizing section of the theme docs at some point. No rush now since we don't have anything setup yet for selecting from a list.
Also, do you think we should actually move the colors.title into this new theme key specifically? So we'd get:
euiTheme.title.weight and euiTheme.title.color
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'm good with that! I'll make that change here in a little bit
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.
Oh actually, spoke too soon. Looks like we can't easily do that because colors has its own custom computed logic plus light vs dark mode settings. We should probably just leave it where it is 🤔
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.
No problem, we can check it out in a later PR.
Co-authored-by: Caroline Horn <[email protected]>
cchaos
left a comment
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've checked through the updated snapshot components for any regressions, didn't find anything too big. I think just one small thing we can do here, the rest will follow up on.
|
jenkins test this |
1 similar comment
|
jenkins test this |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5842/ |
Summary
I strongly recommend following along by commit.
What this PR does:
<EuiTitle />in any caseeuiTextBreakWordCSS 'mixin' that matches what EuiBeacon did witheuiCanAnimateeuiTitle()Sass mixin, that now lives in the component directory and not inglobal_stylingWhat this PR does not do:
euiTitle()Sass mixin and potentially convert them<EuiTitle />(TBD, hopefully my next PR)euiTitle()Sass mixin or$euiTitlesSass variable/JSON mappingsChecklist