[Feature] Range sliders QA fixes#6432
[Feature] Range sliders QA fixes#6432cee-chen merged 31 commits intoelastic:feature/emotion-range-slidersfrom
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6432/ |
|
When testing the following example with This is how it looks in Chrome and how it is supposed to look: |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6432/ |
| )`; | ||
|
|
||
| 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")'; |
There was a problem hiding this comment.
@chandlerprall @constancecchen let me know if there is any security or other type of issue for doing this.
There was a problem hiding this comment.
@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):
|
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
+ fix typo on className
|
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
|
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
|
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
|
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
|
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
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6432/ |
…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>





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
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)yarn compile-scssto update JSON filescalc()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<>)