-
Notifications
You must be signed in to change notification settings - Fork 842
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] Convert EuiCollapsibleNav and EuiCollapsibleNavGroup #6865
Conversation
+ set theme colorMode on all children within `background="dark"`
- doesn't seem to be doing anything or used, so presumably became deprecated in Amsterdam
- `EuiTitle` already sets a font weight of 700, so this isn't really being used meaningfully anywhere
- nest `euiCollapsibleNavGroup__heading` padding as well for consistency and so we can skip adding another Emotion var
… docs - since `background="dark"` now automatically sets the theme
- break from 'playground' example for `EuiCollapsibleGroupNav` to set up schema of different display types (since they're individually fairly different, and affect prop usage substantially)
- 50/50 on this, but it's useful for QA
/* This class does not accept a custom className */ | ||
.euiAccordion__triggerWrapper { | ||
padding: ${euiTheme.size.base}; | ||
} | ||
`, | ||
notCollapsible: css` | ||
/* If the heading is not in an accordion, it needs the padding */ | ||
.euiCollapsibleNavGroup__heading { | ||
padding: ${euiTheme.size.base}; | ||
} |
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.
The notCollapsible
nested selector could technically be flattened out by applying it directly to .euiCollapsibleNavGroup__heading
, but personally I prefer it written this way - it's clearer (at least in my opinion) what the contrasting states are and where the padding needs to be applied.
I'm also being a bit more lax in this component's CSS specificity since we're redesigning the component here shortly.
.euiCollapsibleNavGroup__title, | ||
.euiCollapsibleNavGroup__heading, | ||
.euiAccordion__iconButton { | ||
color: ${euiTheme.colors.ghost}; | ||
} |
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.
See above - several of these selectors could be flattened out (primarily the title/heading selectors), but I personally actually think it makes more sense to keep them combined so that the intent of what we're making a slightly brighter white is all in one place.
Again, I'm also being a bit more lax in this component's CSS specificity since we're redesigning the component, and I think the background
prop may end up going away entirely with the redesign.
// Background colors | ||
none: css``, | ||
light: css` | ||
background-color: ${euiTheme.colors.body}; |
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.
One semi-odd thing to note is that the light
background flips in dark mode but the dark
background does not flip to a light background (it stays dark).
I'm honestly not sure if this was intentional by the original designers, but it's how it behaves currently in prod and I wanted to keep things as 1:1 with prod as possible. Again, it's possible we may scrap the background
prop shortly in the future in any case.
src/components/collapsible_nav/collapsible_nav_group/collapsible_nav_group.stories.tsx
Show resolved
Hide resolved
/** | ||
* Full pattern demo - this was copied from src-docs examples, | ||
* and is meant to be an imitation of Kibana's production nav | ||
*/ |
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 not sure I'm a super huge fan of this story. It feels like it has too much overlap with our docs and I'm not sure if the gain is worth it. Its useful for QA but I'm 50/50 on reverting the commit once QA is over.
Thoughts? Should I go ahead and revert this?
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 agree this might be too close to what's in the docs. I responded to your other comment that maybe removing this one and adding a bit to the primary EuiCollapsibleNav
example might be a good middle ground.
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.
👍 thanks Trevor! I'll go ahead and revert. My only thought is that long-term, we should think about what we're going to do with all our existing docs once we're on EUI+ and Storybook combined. It might make sense then to move everything to Storybook, but right now a duplicated example is not all that helpful.
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 had a time deciding how I thought about this one. I like your suggestion Cee, to long-term move things to Storybook and EUI+
Preview documentation changes for this PR: https://eui.elastic.co/pr_6865/ |
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 left you two comments about the storybook organization. Ran through your QA steps (very helpful) and noticed one difference in prod and staging. Might be unrelated, but felt worth mentioning.
LMK if you have questions or want to discuss comments.
Thanks @cee-chen
src/components/collapsible_nav/collapsible_nav_group/collapsible_nav_group.stories.tsx
Show resolved
Hide resolved
/** | ||
* Full pattern demo - this was copied from src-docs examples, | ||
* and is meant to be an imitation of Kibana's production nav | ||
*/ |
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 agree this might be too close to what's in the docs. I responded to your other comment that maybe removing this one and adding a bit to the primary EuiCollapsibleNav
example might be a good middle ground.
Re the |
This reverts commit c3ced8c.
Preview documentation changes for this PR: https://eui.elastic.co/pr_6865/ |
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!
Thanks Trevor! |
`eui@82.1.0` ⏩ `83.0.0`⚠️ The biggest change in this PR by far is the `EuiButtonEmpty` Emotion conversion, which changes the DOM structure of the button slightly as well as several CSS classes around it. EUI has attempted to convert any custom EuiButtonEmpty CSS overrides where possible, but would super appreciate it if CODEOWNERS checked their touched files. If anything other than a snapshot or test was touched, please double check the display of your button(s) and confirm everything still looks shipshape. Feel free to ping us for advice if not. --- ## [`83.0.0`](https://github.com/elastic/eui/tree/v83.0.0) **Bug fixes** - Fixed `EuiPaginationButton` styling affected by `EuiButtonEmpty`'s Emotion conversion ([#6893](elastic/eui#6893)) **Breaking changes** - Removed `isPlaceholder` prop from `EuiPaginationButton` ([#6893](elastic/eui#6893)) ## [`82.2.1`](https://github.com/elastic/eui/tree/v82.2.1) - Updated supported Node engine versions to allow Node 16, 18 and >=20 ([#6884](elastic/eui#6884)) ## [`82.2.0`](https://github.com/elastic/eui/tree/v82.2.0) - Updated EUI's SVG icons library to use latest SVGO v3 optimization ([#6843](elastic/eui#6843)) - Added success color `EuiNotificationBadge` ([#6864](elastic/eui#6864)) - Added `badgeColor` prop to `EuiFilterButton` ([#6864](elastic/eui#6864)) - Updated `EuiBadge` to use CSS-in-JS for named colors instead of inline styles. Custom colors will still use inline styles. ([#6864](elastic/eui#6864)) **CSS-in-JS conversions** - Converted `EuiButtonGroup` and `EuiButtonGroupButton` to Emotion ([#6841](elastic/eui#6841)) - Converted `EuiButtonIcon` to Emotion ([#6844](elastic/eui#6844)) - Converted `EuiButtonEmpty` to Emotion ([#6863](elastic/eui#6863)) - Converted `EuiCollapsibleNav` and `EuiCollapsibleNavGroup` to Emotion ([#6865](elastic/eui#6865)) - Removed Sass variables `$euiCollapsibleNavGroupLightBackgroundColor`, `$euiCollapsibleNavGroupDarkBackgroundColor`, and `$euiCollapsibleNavGroupDarkHighContrastColor` ([#6865](elastic/eui#6865)) --------- Co-authored-by: Cee Chen <constance.chen@elastic.co> Co-authored-by: Jeramy Soucy <jeramy.soucy@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
EuiCollapsibleNav
to Emotion (very little actually happening here as most of the heavy styles lifting is done byEuiFlyout
)EuiCollapsibleNavGroup
to Emotion (much more going on here - I recommend following along by commit).euiCollapsibleNavGroup--[background]
and.euiCollapsibleNavGroup--withHeading
modifier classesbackground="dark"
should be re-written with EmotioncolorMode="dark"
#6776QA
gh pr checkout 6865 && yarn storybook
background
controls work as expected (in both light and dark mode) forEuiCollapsibleNavGroup
General checklist
Emotion checklist
See #6389