-
Notifications
You must be signed in to change notification settings - Fork 839
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
[Emotion] Organizing files by type vs values and added euiTheme.levels
#5827
Conversation
Apparently altering the theme provider causes all the Emotion snaps to need updating
Preview documentation changes for this PR: https://eui.elastic.co/pr_5827/ |
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! 🎉
I just found one small issue.
|
||
return ( | ||
<> | ||
<ThemeValuesTable |
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.
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.
What would the actual example be though?
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.
Here's an idea: cchaos#55
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.
@miukimiu , I'm going to merge this PR to specifically get these file organization changes in to reduce conflict management. We can follow up with the example snippets.
# Conflicts: # src/global_styling/functions/index.ts # src/global_styling/variables/index.ts # src/services/theme/index.ts # src/services/theme/types.ts # src/themes/amsterdam/global_styling/mixins/shadow.ts # src/themes/amsterdam/global_styling/variables/_typography.ts # src/themes/amsterdam/global_styling/variables/title.ts # src/themes/amsterdam/theme.ts
Preview documentation changes for this PR: https://eui.elastic.co/pr_5827/ |
Any additional work to do in this PR now that the typography PR has merged? Or is this comment outdated? |
Outdated, as I already merged in those changes. But looks like I have another conflict I'll need to resolve locally. |
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; this is a great change!
Jenkins, test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5827/ |
Summary
Primarily the idea is that exports leading with an underscore,
_EuiThemeVars
, are Typescript types and those without are constants,EuiThemeVar
. The values only live within thethemes/Amsterdam/
folder while the top levelglobal_styling/
folder will contain only the theme types & non-value based functions.Checklist
[ ] Checked in mobile[ ] Checked in Chrome, Safari, Edge, and Firefox[ ] 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