[Feature Branch] Convert EuiRange sliders to Emotion and more enhancements #6092
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6092/ |
49 tasks
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6092/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6092/ |
11 tasks
* 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>
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6092/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6092/ |
11 tasks
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6092/ |
* 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>
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6092/ |
4 tasks
* [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>
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6092/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6092/ |
3 tasks
38 tasks
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6092/ |
* 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>
|
@breehall I know this is blocking the release and your Kibana upgrade, so just wanted to give you a quick status update - I'm doing a final pass on the docs page before I merge this in. In particular I'm looking at the playground flyouts - I'm not super happy with their current usability; it feels annoyingly hard to get things in a easy to test state. Once that's done on either Monday or Tuesday, I was hoping to get your and @1Copenut's eyes on this to QA various input states and help test mouse vs keyboard focus, etc. The more bugs we potentially catch pre-release, the fewer backports we have to do! |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6092/ |
- 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
+ optimize via useMemo + address TODO via useEffect + switch to RTL over Enzyme - Enzyme can't process the `useEffect` behavior in render/shallow
+ 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
- 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 `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
…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
- 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
- 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
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6092/ |
|
👋🏾 Here are a few questions / comments I had after QA-ing:
Other than those two things, this looks good. I tested the page with mouse and keyboard controls and keyboard controls successfully as well. |
This is intentional as the
Oh interesting. So if you do this on local, you get a very handy error pane that covers the entire screen:
But on staging/prod the message only shows in console logs:
Either way, these are a series of very intentionally thrown errors (that have unit tests written for them) so I'll write it off for now (working the same as production), but I think we should find some way of wrapping an
Totally agreed! This is already an issue on production it looks like so my preference would be to file an issue + follow up in a separate PR (since changing the width of the input could have unintended effects on the range slider, and @miukimiu may have different ideas on how to resolve the problem). Thanks for the super thorough QA Bree!!! These are all great items I'd love to see us tackle in the future. |
…ed` visual changes in changelog
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6092/ |
jbudz
pushed a commit
to elastic/kibana
that referenced
this pull request
Dec 19, 2022
eui@70.4.0 ⏩ eui@71.0.0 --- ## [`71.0.0`](https://github.com/elastic/eui/tree/v71.0.0) - Implemented new `EuiRange` and `EuiDualRange` designs where the `levels` are now on top of the tracks ([#6092](elastic/eui#6092)) - Added `discuss` and `dotInCircle` glyphs to `EuiIcon` ([#6434](elastic/eui#6434)) - Added `article` glyph to `EuiIcon` ([#6437](elastic/eui#6437)) - Changed the `EuiProvider` usage warnings to not rely on development mode. ([#6451](elastic/eui#6451)) **Breaking changes** - `EuiDualRange` now explicitly requires both `min` and `max` via props types, to match `EuiRange` ([#6092](elastic/eui#6092)) - `EuiRange` and `EuiDualRange`'s `compressed` size no longer impacts track or level sizes, but continues to compress tick and input sizes. ([#6092](elastic/eui#6092)) - Removed all variables for the following components from EUI's theme JSON files: ([#6443](elastic/eui#6443)) - `euiCollapsibleNav*` - `euiColorPicker*` - `euiContextMenu*` - `euiControlBar*` - `euiDataGrid* `(except for z-indices and cell padding sizes) - `euiDatePicker*` - `euiSuperDatePicker*` - `euiDragAndDrop*` - `euiEuiEmptyPrompt*` - `euiFilePicker*` - `euiRange*` - `euiHeaderLinks*` - `euiKeyPad*` - `euiMarkdownEditor*` - `euiResizable*` - `euiSelectable*` - `euiSideNav*` - `euiStep*` - `euiSuggest*` - `euiTable*` (except for color variables) - `euiTooltip*` - `euiButtonFontWeight`, `euiButtonDefaultTransparency`, and `euiButtonMinWidth` - If you were importing any of the above removed JSON variables, we strongly recommend using generic color or sizing variables from `useEuiTheme()` instead. ([#6443](elastic/eui#6443)) **CSS-in-JS conversions** - Converted `EuiRange` and `EuiDualRange` to Emotion; Removed `$euiRangeThumbRadius` ([#6092](elastic/eui#6092)) - Added a new `logicalStyles` utility that automatically converts all non-logical properties in a `style` object to their corresponding logical properties ([#6426](elastic/eui#6426)) - Added a new `logicalShorthandCSS` utility that automatically converts `margin`, `padding`, and other 4-sided shorthands to their corresponding logical properties ([#6429](elastic/eui#6429)) - Added a new `logicalBorderRadiusCSS` utility that automatically converts `border-radius` to corresponding logical properties ([#6429](elastic/eui#6429)) Co-authored-by: Constance Chen <constance.chen@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR converts EuiRange sliders to Emotion and fixes the following issues:
levels#5228The following issues are not going to be fixed in this PR.
PRs that were merged into the feature branch
Changelog
EuiRangeandEuiDualRangedesigns where the levels are now on top of the tracksCSS-in-JS conversions
EuiRangeandEuiDualRangeto EmotionBuild Preview
The build Preview can be found here: https://eui.elastic.co/pr_6092/#/forms/range-sliders
New levels design
QA
Remove or strikethrough items that do not apply to your PR.
General checklist
- [ ] 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)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)- [ ] 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)euiCanAnimate- [ ] Usedgapproperty 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 classes- [ ] If usages exist, consider converting to adataattribute 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<>)