[Emotion] Convert EuiSteps#6555
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6555/ |
a4d73a5 to
333dc1d
Compare
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6555/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6555/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6555/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6555/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6555/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6555/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6555/ |
There was a problem hiding this comment.
Hey Elizabet! Huge apologies, my EuiDataGrid work ended up taking most of my time today and I didn't end up being able to pull this PR down after all. I skimmed it super briefly and did notice a small bug with the titleSize="xs" example.
I'm out tomorrow but will be back on Monday to help address/clean up any code feedback etc before Tuesday's release. Am I cool to push up to this PR?
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6555/ |
|
@cee-chen I went through your review and pushed some changes.
Yes, no problem at all. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6555/ |
- remove unnecessary `isDisabled` style - it's not doing anything - clean up lineGradient readability - add `euiStepTitleStyles` to avoid nesting CSS selectors - Remove ambiguous `small` vs `medium` sizes: pass the equivalent props instead, and rename the size variable to be clear that it's referring to the `xs` size, not the `s` size - use more logicalCSS (easier to read) and less logicalShorthandCSS (it's getting overridden anyway) + fix `medium` sized titles getting padding that they should not have been + bump up padding-top on `s` titles to center text a bit more - fix EuiStepNumber not actually having the default size that its props documentation claims it does
- remove unnecessary repeated `makeLineProgress` - it's defined once in `euiStepHorizontal` and it doesn't need to be redefined again - remove `:not` isDisabled class selector in favor of an `enabled` style - remove unused isComplete/isIncomplete styles - pull out separate euiStepHorizontalNumberStyles fn and remove unnecessary selector nesting on box shadow application (if it's selected, it can't also be loading/disabled/warning) - make emotion classNames more closely match passed `status`es - fix BEM class naming - Remove old prop documentation to now-removed `isSelected` and `isComplete` props, + remove `isSelected` var naming in favor of `isCurrent` - clean up unnecessarily verbose `isCurrent` spread operator
- create `euiStepNumberContentStyles` to reduce CSS nesting - rewrite JSX output to use a switch (w/ groupings for icon vs number types) and an aria labels map for clearer readability - move `createStepsNumber` to fn within map its used in (since it's fairly basic and only used for one specific purpose, there's no need to keep passing around euiTheme - prefer theme vars over static `px` sizes
+ fix missing specific titles for `danger` and `loading` statuses
| // Remove the respective lines if the first or last child | ||
| &:first-of-type > .euiStepHorizontal::before, | ||
| &:last-of-type > .euiStepHorizontal::after { | ||
| display: none; | ||
| } |
There was a problem hiding this comment.
Note for reference: I debated trying to flatten this by passing isFirst or isLast to the underying EuiStep, but decided against it - I don't think the extra code cruft is worth the slight CSS flattening, as consumers are unlikely to want to override this CSS
cee-chen
left a comment
There was a problem hiding this comment.
Hey Elizabet, done with my QA & cleanups and I think this is GTG. I just have one remaining comment I found while comparing prod vs dev, but I'm not sure if the change was intentional.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6555/ |
| onClick={onStepClick} | ||
| disabled={disabled} | ||
| css={cssStyles} | ||
| data-step-status={status} |
There was a problem hiding this comment.
Note for reference: I added this new data-step-status util because we're not setting status classes via Emotion, and because it looks like Fleet is hooking into euiStepHorizontal-isIncomplete as an override (link).
If needed we can add this data attr to EuiStep as well, but I currently don't see the need / any Kibana usage
- remove unnecessary Fragments and divs
| current: css` | ||
| ${euiShadowXSmall(euiThemeContext)} | ||
| `, |
There was a problem hiding this comment.
I see the CSS for this correctly appearing, but I can barely see this shadow at all on either light or dark mode. Am I just blind, or should we consider removing this? 🤷 @miukimiu, any opinions?
There was a problem hiding this comment.
I could notice it in light mode. But I agree, it's almost unperceivable. So I removed it.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6555/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6555/ |
## Summary `eui@75.0.0` ⏩ `eui@75.1.0` --- ## [`75.1.0`](https://github.com/elastic/eui/tree/v75.1.0) - Added padding to `EuiStep` title to better align with icon ([#6555](elastic/eui#6555)) - Added a new `lineNumbers.annotations` API to `EuiCodeBlock`. This new feature displays an informational icon next to the specified line number(s), providing more context via popover ([#6580](elastic/eui#6580)) **Bug fixes** - Fixed bug in `EuiRange` where styles were applied incorrectly when custom ticks were passed but `showTicks` were false ([#6588](elastic/eui#6588)) - Fixed `fleetApp` and `agentApp` icons that were swapped ([#6590](elastic/eui#6590)) **CSS-in-JS conversions** - Converted `EuiSteps` to Emotion; Removed `$euiStepStatusColorsToFade`, `$euiStepNumberSize`, `$euiStepNumberSmallSize`, and `$euiStepNumberMargin` ([#6555](elastic/eui#6555))
Summary
This PR converts EuiSteps to Emotion (closes #6413) and closes #5847.
Design enhancements
QA
Remove or strikethrough items that do not apply to your PR.
General checklist
@defaultif default values are missing) and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] Checked for breaking changes and labeled appropriatelyEmotion conversion checklist
withEuiThemeneed to passtrueas the second argument to itspropUtilityForPlaygroundinsrc-docs/src/views/{component}/playground.js)shouldRenderCustomStyles()test was added and passes with parent component and any nestedchildProps(e.g.tooltipProps)- [ ] Removed anymount()ed snapshots in favor ofrender()or a more specific assertion$euiSizetoeuiTheme.size.base)calc()tomathWithUnitsif possible (if mixing different unit types, this may not be possible)- [ ] Added an@warndeprecation message within theglobal_styling/mixins/{component}.scssfilesrc/components/index.scsssrc/amsterdam/overrides/{component}.scssfiles (styles within should have been converted to the baseline Emotion styles)euiCanAnimategapproperty to add margin between items if using flex-inlineand-blocklogical properties (check inline styles as well as CSS)euiComponent,euiComponent__child){euiComponent}-(case sensitive) to check for source code usage of modifier classesdataattribute so that consumers still have something to hook into**/target, **/*.snap, **/*.storyshotfor less noise) foreui{Component}(case sensitive) to find:euiBadgeclass on a div instead of simply using theEuiBadgecomponent).jsdocs files to TSfrom '../src', replace<React.Fragment>with<>)- [ ] Optional component/code cleanup: consider splitting up the component into multiple children if it's overly verbose or difficult to reason about