Skip to content

[Feature] Range sliders QA fixes#6432

Merged
cee-chen merged 31 commits intoelastic:feature/emotion-range-slidersfrom
elizabetdev:emotion-range-sliders-qa-fixes
Dec 3, 2022
Merged

[Feature] Range sliders QA fixes#6432
cee-chen merged 31 commits intoelastic:feature/emotion-range-slidersfrom
elizabetdev:emotion-range-sliders-qa-fixes

Conversation

@elizabetdev
Copy link
Copy Markdown
Contributor

@elizabetdev elizabetdev commented Nov 23, 2022

Summary

This PR is basically me going through the QA to-do list. This PR merges into the Range Sliders Feature Branch: #6092.

Build Preview

The build Preview can be found here: https://eui.elastic.co/pr_6432/#/forms/range-sliders

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 ran yarn compile-scss to update 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 added the skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) label Nov 23, 2022
@kibanamachine
Copy link
Copy Markdown

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

@elizabetdev
Copy link
Copy Markdown
Contributor Author

When testing the following example with fullWidth={true} in Safari the stripes don't look good:

Screenshot 2022-11-23 at 17 54 26

This is how it looks in Chrome and how it is supposed to look:

Screenshot 2022-11-23 at 17 58 14

@kibanamachine
Copy link
Copy Markdown

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

@elizabetdev
Copy link
Copy Markdown
Contributor Author

I decided to use URL-encoded SVGS to fix the issue mentioned before.

Now in Safari the stripes look good.

svgs-stripes@2x

)`;

const stripesLightEncodedSVGUrl =
'url("data:image/svg+xml, %3Csvg%20fill%3D%22none%22%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%20viewBox%3D%220%200%2024%206%22%3E%0A%20%20%3Crect%20width%3D%2224%22%20height%3D%226%22%20fill%3D%22%23fff%22%20fill-opacity%3D%22.5%22%2F%3E%0A%20%20%3Cpath%20fill-rule%3D%22evenodd%22%20clip-rule%3D%22evenodd%22%20d%3D%22m12%206%206-6h-3l-3%203v3ZM18%206V3l-3%203h3ZM6%206l6-6H9L6%203v3Z%22%20fill%3D%22%23fff%22%20fill-opacity%3D%22.5%22%2F%3E%0A%20%20%3Cpath%20fill-rule%3D%22evenodd%22%20clip-rule%3D%22evenodd%22%20d%3D%22M12%206V3L9%206h3ZM0%206l6-6H3L0%203v3ZM6%206V3L3%206h3ZM18%206l6-6h-3l-3%203v3ZM24%206V3l-3%203h3Z%22%20fill%3D%22%23fff%22%20fill-opacity%3D%22.5%22%2F%3E%0A%3C%2Fsvg%3E")';
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@chandlerprall @constancecchen let me know if there is any security or other type of issue for doing this.

Copy link
Copy Markdown
Contributor

@cee-chen cee-chen Dec 2, 2022

Choose a reason for hiding this comment

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

@miukimiu Can you double check what version of Safari you're on? I'm on latest 16.1 and I don't see the linear gradient being an issue:

I'm going to revert this change and have someone else confirm they can't repro.

edit: I'm a dingdong, I forgot the fullWidth bit. I played around with this more and was able to find a decent workaround by using percentages for the gradient stops and background-size (technique courtesy of css-tricks):

@elizabetdev elizabetdev marked this pull request as ready for review November 24, 2022 13:20
@kibanamachine
Copy link
Copy Markdown

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

- remove need for modifier class + update snapshots

- prefer not to pass `fullWidth` as an arg into param - let there be a 2nd max-width property override instead
@kibanamachine
Copy link
Copy Markdown

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

- existing Kibana selectors can use `[class*="euiRangeLevels-color"]` instead
if values can only be left or right, there's no point to targeting both - just target the main `__` class
@kibanamachine
Copy link
Copy Markdown

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

- remove unnecessary border CSS (not sure what it's doing?) and add clarifying CSS comment

- simplify various Emotion conditionals to ternaries
@kibanamachine
Copy link
Copy Markdown

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

- We no longer support IE and Edge does not appear to need this styling
- most notable one: we're calling small `2px`-based border widths all over the place, which should get its own `thumbBorderWidth` var

+ reorder several properties - prefer display/flow properties first, then color/appearance
- remove `transition` - `box-shadow` doesn't even exist on the tooltip and `transform` is not meaningfully changing

- remove both double pseudo element selectors - there isn't a need for 2 elements to represent 1 arrow
+ combine just `::before` selector

- DRY out various CSS being applied to both left and right CSS - if it's being applied to both, it can just go on the base CSS

- tweak positioning of tooltip arrow - 1px offset does not appear to be needed any longer

- DRY out use of `euiFontSize`

+ reorder several properties - prefer display/flow properties first, then color/appearance
- Fix `selected` styling never actually correctly applying (already occurring in prod)

+ remove duplicate/unused border-radius property

+ remove duplicate `::before` pseudo selector CSS - it's already being set in `hasPseudoTickMark` via `tickStyles`
- the black focus color does not match the primary color we use for the rest of the thumb/track
- 2px is what's on prod, not 4px - i think this is supposed to line up with the border width?
+ make stripe size dynamic to match custom border width changes
@kibanamachine
Copy link
Copy Markdown

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

see https://css-tricks.com/stripes-css/#aa-funky-town - using percentages and then `background-size` appears to solve the issue
- other focus selectors are not having any visible difference whatsoever in latest FF/Edge/Safari
@kibanamachine
Copy link
Copy Markdown

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

- now that we no longer have to support IE and can use `:focus-visible` on its own, there's a whole lot of cruft we can remove, including JS `hasFocus` tracking (vs CSS built-in focus)

- Move highlight range to last DOM item- some CSS used in place of the JS `hasFocus` requires the `~` sibling selector

- make the focus color a variable so that it can be changed in one place if needed

+ clean up reset styles - half of them aren't needed (tested in FF+Edge+Safari)
- if it's not being used in more than one place, it doesn't really need to be a fn/isn't DRYing anything out
- it's not doing anything that I can tell with prepend/append
- remove not-especially-helpful comments

- remove unnecessary syntax

- remove unnecessary !important

- DRY out logical properties usage applicable
@cee-chen cee-chen merged commit 47d15d2 into elastic:feature/emotion-range-sliders Dec 3, 2022
@kibanamachine
Copy link
Copy Markdown

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

cee-chen added a commit that referenced this pull request Dec 6, 2022
…ments (#6092)

* Merged range sliders amsterdam and legacy styles (#6084)

* [Emotion] Convert range sliders (#6116)

* Adding all necessary emotion style files

* WIP styles

* Converted range_label

* Converted range mixins

* Converted dual_rangee

* Converted range_draggable

* Added range_highlight

* Converted range_thumb

* Converted range_tooltip

* Removed EuiRangeTooltip compressed prop

* Moved euiFormControlSize mixin to form.styles.ts

* Added euiFormVariables

* Converted range_slider

* Updating snapshots with 2 tests failing

* sasslint fix

* mark min and max as having default values

* selector fix

* compressed types fix

* snapshot updates

* Fixing range track top position

* Revert ticks example

* Fix logical css

* Removing unnecessary styles

* Remove trailing slash from import statements, update dtsgenerator script to remove trailing slashes from imports

* Converting px to variables and adding `shouldRenderCustomStyles`

* Adding custom thumbs so that we can have z-index and put them above levels

* Adding aria label on custom thumb

* use default input thumb

* Improving comments. Some styles fixes.

* Adressing PR review

Co-authored-by: Greg Thompson <thompson.glowe@gmail.com>
Co-authored-by: Chandler Prall <chandler.prall@gmail.com>

* [Range sliders] Implement new levels design (#6265)

* Adding liner gradient on top of levels

* Styling highlight with levels

* Fix level range not showing for single ranges

* thumb colors match levels

* update snapshot

* Fix focus issue

* Fixed levels position when no ticks are available

* Update src/components/form/range/range_levels.styles.ts

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

* Adressing PR review

* Converting some range highlight inline styles to Emotion

* Getting levels colors in thumb as `currentcolor`

* [misc] DRY out color array

* [cleanup] DRY out various consts, types, & utils around range level colors to new file

- nb: separate file is required for webpack to not throw a fit over circular dependencies

* [more cleanup] move new level color util to new file

* [misc linting] remove unnecessary newlints from single-line strings

+ remove extra semicolons, since the utils already contain ending semicolons

Co-authored-by: Greg Thompson <thompson.glowe@gmail.com>
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
Co-authored-by: Constance Chen <constance.chen.3@gmail.com>

* [Range sliders docs] Add non linear intervals example (#6330)

* [Range sliders docs] Add non linear example

* Changing examples source types to TSX

* [PR feedback] Remove typescript need for any

- using `event.currentTarget` over `target` solves most of the single range issues

+ refactor single instances of callbacks to be inline instead of instantiating a var

Co-authored-by: Constance Chen <constance.chen.3@gmail.com>

* Adding CL

* [Feature] Range sliders QA fixes (#6432)

* Fix tests and `fullWidth`

* Simplified `calc()` to `mathWithUnits`

* Wrapped atransition in `euiCanAnimate`

* Replaced `<Fragment/>` with `</>`

* Removed unnecessary class names

* Fixing but with `repeating-linear-gradient` in Safari

* Encoded SVGs again to avoid masks than could lead to possible IDs conflicts

* `fullWidth` PR feedback

- remove need for modifier class + update snapshots

- prefer not to pass `fullWidth` as an arg into param - let there be a 2nd max-width property override instead

* Explain why style is being skipped

* [PR feedback] Remove need to pass `disabled` to inner style fn

+ fix typo on className

* Remove unnecessary `classNames()` around static classes

* Remove `.euiRangeLevels--` modifier classes

- existing Kibana selectors can use `[class*="euiRangeLevels-color"]` instead

* Remove unnecessary `.euiRangeTooltip__value` modifier class

if values can only be left or right, there's no point to targeting both - just target the main `__` class

* Fix overflow-x/y on non-FF browsers

* Clean up pseudo tick CSS

- remove unnecessary border CSS (not sure what it's doing?) and add clarifying CSS comment

- simplify various Emotion conditionals to ternaries

* Convert various inline style positioning to logical properties

* Clean up unnecessary `calc`s in favor of `mathWithUnits`

* Remove unused `::-ms-track` browser CSS

- We no longer support IE and Edge does not appear to need this styling

* DRY out various static `px` width

- most notable one: we're calling small `2px`-based border widths all over the place, which should get its own `thumbBorderWidth` var

+ reorder several properties - prefer display/flow properties first, then color/appearance

* DRY out EuiRangeTooltip CSS

- remove `transition` - `box-shadow` doesn't even exist on the tooltip and `transform` is not meaningfully changing

- remove both double pseudo element selectors - there isn't a need for 2 elements to represent 1 arrow
+ combine just `::before` selector

- DRY out various CSS being applied to both left and right CSS - if it's being applied to both, it can just go on the base CSS

- tweak positioning of tooltip arrow - 1px offset does not appear to be needed any longer

- DRY out use of `euiFontSize`

+ reorder several properties - prefer display/flow properties first, then color/appearance

* EuiRangeTicks - fixes & cleanup

- Fix `selected` styling never actually correctly applying (already occurring in prod)

+ remove duplicate/unused border-radius property

+ remove duplicate `::before` pseudo selector CSS - it's already being set in `hasPseudoTickMark` via `tickStyles`

* Restore thumb focus color to prod appearance

- the black focus color does not match the primary color we use for the rest of the thumb/track

* Fix tick marks not being correctly centered on the thumb

- 2px is what's on prod, not 4px - i think this is supposed to line up with the border width?

* Revert SVG stripes change

+ make stripe size dynamic to match custom border width changes

* Actually fix stripe gradient issue for Safari

see https://css-tricks.com/stripes-css/#aa-funky-town - using percentages and then `background-size` appears to solve the issue

* Clean up draggable CSS

- other focus selectors are not having any visible difference whatsoever in latest FF/Edge/Safari

* Massively clean up focus styles

- now that we no longer have to support IE and can use `:focus-visible` on its own, there's a whole lot of cruft we can remove, including JS `hasFocus` tracking (vs CSS built-in focus)

- Move highlight range to last DOM item- some CSS used in place of the JS `hasFocus` requires the `~` sibling selector

- make the focus color a variable so that it can be changed in one place if needed

+ clean up reset styles - half of them aren't needed (tested in FF+Edge+Safari)

* Remove functionally unused `euiRangeTrackSize` fns

- if it's not being used in more than one place, it doesn't really need to be a fn/isn't DRYing anything out

* Remove unused group CSS

- it's not doing anything that I can tell with prepend/append

* Misc CSS cleanup

- remove not-especially-helpful comments

- remove unnecessary syntax

- remove unnecessary !important

- DRY out logical properties usage applicable

Co-authored-by: Constance Chen <constance.chen.3@gmail.com>

* Changelog update to note removed Sass vars

- most vars/mixins were left intact due to `EuiColorStop` usage - they should be removed after either EuiColorStop is migrated or all EuiForm components are migrated

* [Perf/tech debt] Convert EuiRangeTrack to function component

+ optimize via useMemo

+ address TODO via useEffect

+ switch to RTL over Enzyme - Enzyme can't process the `useEffect` behavior in render/shallow

* [Perf/tech debt] Memoize EuiRangeLevels

+ split out logic into internal only sub-component with its own memoization logic

+ split out validation fn to separate const to match other files

+ switch to RTL over Enzyme - Enzyme can't process the `useEffect` behavior in render/shallow

* [Perf] Memoize objs etc. + FC instances of `logicalStyles()` where possible

* DRY out repeated `trackWidth` ref logic to just a single passed prop

- useCallback doesn't work on refs / causes them to stop working, so let's improve perf by not watching/setting state on 3 diff nodes within EuiRange

- make `EuiRangeTrack` in charge of the single ref and pass the value down as an optional render fn

- I strongly recommend turning off whitespace changes when viewing this diff

* Fix/clean up passing of `compressed` prop

- Fix `EuiRangeWrapper` to actually change height on `compressed` flag

- `EuiRangeLevels`, `EuiRangeHighlight`, `EuiRangeSlider`, and `EuiRangeDraggable` do nothing with `compressed` and don't need to be passed the prop

- in the case of `EuiRangeTicks`, compressed is used but is already defined in the ...rest spread and initial types def and doesn't need to be re-defined

* SQUASH WITH Memoize EuiRangeLevels

* [Docs][Types] DRY out many repeated types + more thoroughly document prop behavior

- We're repeating a lot of the same types over and over and if our types ever change for whatever reason it's going to be annoying to update 5+ files

- More importantly, we're also (incorrectly) repeating type/prop docs between both EuiRange and EuiDualRange and this way / putting them in one place makes it much easier to compare and contrast docs

- NOTE: Kibana imports of `EuiRangeTicks` should now import directly from `@elastic/eui` and not reach into the component

+ DRY out EuiDualRange tests & remove `requiredProps` where it's not relevant ot the test

* [EuiDualRange] Require `min` and `max` props

- it's not clear to me why `min`/`max` isn't required on EuiDualRange when it's documented as such & has the same default behavior as EuiRange

+ clean up tests

* [Docs] Improve playground experience for EuiRange & EuiDualRange

- currently there's a whole bunch of errors thrown if you just try to do something like toggle `showTicks` - this should resolve that frustrating experience

- the types cleanup in a previous commit should also have resolved many unnecessary/incorrect props showing up in the playground

* [PR feedback] Fix compressed input thumb position; document `compressed` visual changes in changelog

Co-authored-by: Greg Thompson <thompson.glowe@gmail.com>
Co-authored-by: Chandler Prall <chandler.prall@gmail.com>
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
Co-authored-by: Constance Chen <constance.chen.3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants