[Emotion ] Convert EuiColorStops#6489
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6489/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6489/ |
| block-size: ${range.thumbHeight}; | ||
| margin-block-start: ${mathWithUnits(range.thumbHeight, (x) => x * -0.5)}; | ||
|
|
||
| .euiColorStopThumbPopover__anchor { |
There was a problem hiding this comment.
I don't think I can pass anchor props to the anchor. I just have the option to pass a className. Is it ok to do this way? Or should I improve the popover to accept anchor props so that we can pass a css prop?
There was a problem hiding this comment.
Yeah it looks like we need to add anchorProps to EuiPopover instead of just anchorClassName. IMO this workaround (nesting CSS) is fine for now; I'd look at anchorProps as a separate PR personally
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6489/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6489/ |
|
I hate to do this but: there isn't a single usage in Kibana of this component. Why do we need it? Can we consider removing the component if there isn't a use for it, reducing unnecessary code and style complexity? cc @daveyholler for thoughts |
|
I think that's a totally fair question. Are there any valid open requests, enhancement or otherwise, that are related to this component? Personally, I'm all for removing the things that don't get used. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6489/ |
| export { keys }; | ||
|
|
There was a problem hiding this comment.
My 2c (not a blocking change request): I'm not a fan for alphabetizing just for the sake of it. Associations/groups of certain types of logic are more important than alphabetical order, just IMO. For instance, this comment now makes less sense with the export line super far away from it.
- border-color seems to be unused, removing that instead
|
|
||
| const [trackWidth, setTrackWidth] = useState(0); | ||
|
|
||
| const classes = classNames('euiRangeTrack', className); |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6489/ |
- they aren't working in any case because classes have been rmeoved
- remove need for passing `isDisabled` - remove unnecessary not/focus-visible and Safari workarounds - focus-visible is now supported enough that this isn't an issue - fix non-working focus styles - `hexToRgb` returns an array and needs to be correctly interpolated to a comma separated string - order styles by state and element
- remove unnecessary box-shadow - it's not working and when fixed to work, it doesn't match production - remove unnecessary border - already inheriting that from EuiRangeThumb - don't show the popover while dragging - Show the focus ring when the popover is open (to match focus ring when being clicked/dragged) - honestly not totally sure why I'm bothering, this component feels so buggy and laggy 🙈
cee-chen
left a comment
There was a problem hiding this comment.
I QA'd in Safari, FF, and Chrome, and honestly (separate from this conversion) this component has a lot of lag/responsiveness issues with mouse interactions (although keyboard interactions are solid). Popovers don't always close automatically on outside click and become stuck/impossible to close, don't react to the escape key, etc 🙈 I really think this is a good candidate for deprecation personally.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6489/ |
## Summary `eui@72.0.0` ⏩ `eui@72.1.0` --- ## [`72.1.0`](https://github.com/elastic/eui/tree/v72.1.0) - Changed design of empty ranges in `EuiColorStops` to have diagonal gray stripes instead of a solid light gray color ([#6489](elastic/eui#6489)) - Changed popover in `EuiColorStops` to not appear when dragging/moving a color stop ([#6489](elastic/eui#6489)) - `EuiPopover` now supports overriding `focusTrapProps.onClickOutside`, which allows customization of auto-close behavior on outside click. ([#6500](elastic/eui#6500)) **CSS-in-JS conversions** - Converted `EuiColorStops` to Emotion ([#6489](elastic/eui#6489)) - Added `brighten` service to manipulate CSS-in-JS colors ([#6489](elastic/eui#6489))
Summary
This PR converts
EuiColorStopsto Emotion and adds a diagonal stripes background to empty ranges.The idea behind the stripes is to distinguish when it is an empty range. A few users were thinking that the color gray would actually mean the representation of a color stop. As we can see below on the fixed color segments example if users passed a gray as a color stop it could seem an empty range.
Closes #2876
I updated the range slider levels stripes to be more in sync with the
EuiColorStopsempty ranges. I also think it looks better with less spaced stripes.QA
Remove or strikethrough items that do not apply to your PR.
General checklist
Emotion 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)mount()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)@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<>)