Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Emotion ] Convert EuiColorStops #6489

Merged
merged 15 commits into from
Dec 21, 2022
Merged

[Emotion ] Convert EuiColorStops #6489

merged 15 commits into from
Dec 21, 2022

Conversation

elizabetdev
Copy link
Contributor

@elizabetdev elizabetdev commented Dec 15, 2022 •
edited
Loading

Summary

This PR converts EuiColorStops to 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

color-stops@2x

I updated the range slider levels stripes to be more in sync with the EuiColorStops empty ranges. I also think it looks better with less spaced stripes.

range-slider-levels@2x

QA

Remove or strikethrough items that do not apply to your PR.

General checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

Emotion conversion checklist

  • Does it work?
  • Output CSS matches the previous CSS / as expected in browsers
  • Rendered className reads as expected in snapshots and in browsers
  • Checked component playground (class components wrapped in withEuiTheme need to pass true as the second argument to its propUtilityForPlayground in src-docs/src/views/{component}/playground.js)
     
  • Unit tests
  • shouldRenderCustomStyles() test was added and passes with parent component and any nested childProps (e.g. tooltipProps)
  • Removed any mount()ed snapshots in favor of render() or a more specific assertion
     
  • Sass/Emotion conversion process
  • Converted all global Sass vars/mixins to JS (e.g. $euiSize to euiTheme.size.base)
  • Removed or converted component-specific Sass vars/mixins to exported JS versions, listed removals in changelog, and manually updated our theme JSON files
  • Simplified calc() to mathWithUnits if possible (if mixing different unit types, this may not be possible)
  • Added an @warn deprecation message within the global_styling/mixins/{component}.scss file
  • Removed component from src/components/index.scss
  • Deleted any src/amsterdam/overrides/{component}.scss files (styles within should have been converted to the baseline Emotion styles)
     
  • CSS tech debt
  • Reduced specificity where possible (usually by reducing nesting and class name chaining)
  • Wrapped all animations or transitions in euiCanAnimate
  • Used gap property to add margin between items if using flex
  • Converted side specific padding, margin, and position to -inline and -block logical properties (check inline styles as well as CSS)
     
  • DOM cleanup
  • Did not remove any block/element classNames (e.g. euiComponent, euiComponent__child)
  • Deleted any modifier classNames or maps if not being used in Kibana. Before doing this step:
    • Search/grep through Kibana's codebase for {euiComponent}- (case sensitive) to check for source code usage of modifier classes
    • If usages exist, consider converting to a data attribute so that consumers still have something to hook into
       
  • Kibana due diligence
  • Pre-emptively check how your conversion will impact the next Kibana upgrade. This entails searching/grepping through Kibana (excluding **/target, **/*.snap, **/*.storyshot for less noise) for eui{Component} (case sensitive) to find:
  • Any test/query selectors that will need to be updated
  • Any Sass or CSS that will need to be updated, particularly if a component Sass var was deleted
  • Any direct className usages that will need to be refactored (e.g. someone calling the euiBadge class on a div instead of simply using the EuiBadge component)
     
  • Extras/nice-to-have
  • Documentation pass:
    • Converted any remaining .js docs files to TS
    • Misc cleanup of docs code (e.g. combine imports to single from '../src', replace <React.Fragment> with <>)
  • Check for issues in the backlog that could be a quick fix for that component
  • Optional component/code cleanup: consider splitting up the component into multiple children if it's overly verbose or difficult to reason about

@elizabetdev elizabetdev mentioned this pull request Dec 15, 2022
Closed
31 tasks
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6489/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6489/

elizabetdev
elizabetdev commented Dec 19, 2022
View reviewed changes
src/components/color_picker/color_stops/color_stop_thumb.styles.ts
block-size: ${range.thumbHeight};
margin-block-start: ${mathWithUnits(range.thumbHeight, (x) => x * -0.5)};

.euiColorStopThumbPopover__anchor {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6489/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6489/

@elizabetdev elizabetdev marked this pull request as ready for review December 19, 2022 19:51
@cee-chen
Copy link
Contributor

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

@daveyholler
Copy link
Contributor

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.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6489/

cee-chen
cee-chen reviewed Dec 21, 2022
View reviewed changes
src/services/index.ts
Comment on lines -11 to -12
export { keys };

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cee-chen
Fix missing range color
69d0bef 
- border-color seems to be unused, removing that instead
cee-chen
cee-chen reviewed Dec 21, 2022
View reviewed changes
src/components/form/range/range_track.tsx
@@ -110,9 +112,11 @@ export const EuiRangeTrack: FunctionComponent<EuiRangeTrackProps> = ({

const [trackWidth, setTrackWidth] = useState(0);

const classes = classNames('euiRangeTrack', className);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch on this!

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6489/

@cee-chen
Replace :not selectors with JS logic
b1b3a1a 
- they aren't working in any case because classes have been rmeoved
@cee-chen
[PR feedback] More styles cleanup
3643a9f 
- 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
@cee-chen
Improve popover/drag behavior of color thumbs
28aa89c 
- 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
changelog
1f876f7
@cee-chen
[docs] fix ts-ignore
d4cefef
@cee-chen cee-chen enabled auto-merge (squash) December 21, 2022 19:20
cee-chen
cee-chen approved these changes Dec 21, 2022
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6489/

@cee-chen cee-chen merged commit 737ac3d into elastic:main Dec 21, 2022
@1Copenut 1Copenut mentioned this pull request Jan 3, 2023
Merged
1Copenut added a commit to elastic/kibana that referenced this pull request Jan 4, 2023
@1Copenut
Upgrade EUI to v72.1.0. (#148354)
9a0a72c 
## 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))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cee-chen cee-chen cee-chen approved these changes

Assignees
No one assigned
Labels
emotion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[EuiColorStops] Beginning from empty should look more empty
4 participants
@elizabetdev @kibanamachine @cee-chen @daveyholler