SliderControl: Create new control using imported G2 Slider component #42315
Conversation
aaronrobertshaw
added
the
[Feature] UI Components
|
Size Change: +27 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
aaronrobertshaw
force-pushed
the
add/slider-component
branch
from
0eb175f
to
3a9712e
Compare
aaronrobertshaw
changed the title
|
From #40507
I'm currently working on the new |
There was a problem hiding this comment.
Hey @aaronrobertshaw , thank you for working on this! I started having a preliminary look and left a few comments around.
One thing that came to mind is if this component should be wrapped with BaseControl, which would add support for label and help text
From what I can see, the component is already looking quite good! We may still need to implement a few features from RangeSlider :
- marks
stepprop- labels and help text
- resetFallbackValue (although we may argue it may be a bit redundant)
- tooltip support
| let next = `${ nextValue }`; | ||
|
|
||
| if ( initialUnit ) { | ||
| next = createCSSUnitValue( nextValue, initialUnit ); |
There was a problem hiding this comment.
Shouldn't slider only deal with unitless values?
There was a problem hiding this comment.
The G2 Slider dealt with units so I maintained that approach here.
Only dealing with unitless values would simplify this component in favour of moving that logic to any other component that uses the Slider and units in its values. Is there value in keeping that logic here so it is consistent for all consumers of Slider?
There was a problem hiding this comment.
In my views, SliderControl should deal with unitless values.
Any other component that components SliderControl together with other input controls dealing with units (e.g UnitControl) will have to keep the numeric part in sync with SliderControl internally (similarly to what UnitControl does internally with NumberControl)
| isFocused && styles.focused( colors ), | ||
| error && isFocused && styles.focusedError, |
There was a problem hiding this comment.
Could we use CSS pseudo-selectors instead of JS logic for focused and focused error states?
There was a problem hiding this comment.
I did question this initially but ended up importing this faithfully from the G2 implementation of the Slider.
Today, I've dug deeper into why this approach was taken. In Safari, I ran into problems getting it to reflect the focus state when solely relying on pseudo-selectors.
It appears that there is something funky with its shadow DOM for the range input. If you toggle on the :focus state for the input via Safari's dev tools, you'll see the correct focus styling. Simply interacting with the slider, however, doesn't seem to result in the input matching the focused state.
This JS logic does achieve the consistent focus styling for me across browsers.
| } ); | ||
| const [ value, initialUnit ] = parseCSSUnitValue( `${ _value }` ); | ||
|
|
||
| const id = useFormGroupContextId( idProp ); |
There was a problem hiding this comment.
Should this used across the package for all control related components? If so, can it be promoted outside of the ui folder?
There was a problem hiding this comment.
The FormGroup readme say it's experimental at the moment. I'm not sure if it is ready for wider use but wanted to stay as close to the G2 version as possible using what was already available. Would this be better addressed in a follow-up?
There was a problem hiding this comment.
I gave FormControl a deeper look — it seems like it partially overlaps with BaseControl.
I would probably switch to BaseControl for the time being, and later have a look at FormGroup and at the chance of consolidating BaseControl and FormGroup into one component (this may be also interesting / useful in the context of the recent challenge of labelling compound components that we're facing with BorderBoxControl)
| * | ||
| * @return {number} The interpolated value. | ||
| */ | ||
| export function interpolate( |
There was a problem hiding this comment.
Should we add unit tests for interpolate and interpolateRounded ?
There was a problem hiding this comment.
I'll write some unit tests for these and have them available early next week. I've run out of time for today.
|
Thanks for the early feedback @ciampo, it's always super helpful 🙇
My apologies for not being clearer in the PR description or my earlier comment. When I noted the intent of this component to be a "low-level internal slider input", it was that this component would encapsulate just the Another part of my thinking was keeping this A further breakdown of how I saw these slider-related controls is:
|
|
Thank you for clarifying!
After discussing with @mirka, and referring to the roadmap outlined in #40507, we don't really see the utility of having Also, we don't think that The remaining features of As we will work on these higher-level components, we will have a chance at discussing a few aspects:
|
| * | ||
| * @default 'default' | ||
| */ | ||
| size?: 'small' | 'default' | 'large'; |
There was a problem hiding this comment.
Ugh I'm having a hard time deciding what to do with sizes in this one. We can kind of start with a clean slate. I'm thinking default=36 and __unstable-large=40. Do you think we can get away with not adding a 30px variant at all? I'm hoping so, because all our basic components now have at least a 36px variant (#39397).
Also we can basically ignore the controlHeight* variables in CONFIG — those are not relevant anymore and are due for removal.
aaronrobertshaw
force-pushed
the
add/slider-component
branch
from
9acaa77
to
b1fd489
Compare
aaronrobertshaw
changed the title
aaronrobertshaw
force-pushed
the
add/slider-component
branch
from
ab9096c
to
b1a5c60
Compare
aaronrobertshaw
force-pushed
the
add/slider-component
branch
from
b1a5c60
to
b38845a
Compare
aaronrobertshaw
force-pushed
the
add/slider-component
branch
2 times, most recently
from
d0b97ff
to
82b8657
Compare
aaronrobertshaw
force-pushed
the
add/slider-component
branch
from
82b8657
to
65edf54
Compare
|
Just a quick update to note there is ongoing work to clean up the NumberControl types around the value prop that the new Slider controls would benefit from. I'll dust off this PR and the follow-ups for the SliderNumberControl and SliderUnitControl once #45982 lands. This most likely will not be until the new year. |
|
Absolutely — I've been AFK for the past month. I'll finish catching up and resume work on #45982 ASAP |
|
Going to close this PR for now. It is still dependent on #45982 but can be resurrected if needed. |
Related:
What?
Creates a new
SliderControlcomponent representing only the slider part of theRangeControli.e. no number input field, no icons etc.Why?
This is a step toward better, more modular slider-related controls.
How?
interpolateutils from G2BaseControlfor labelling purposes etc.SliderControlhas a defaultsizeof 36px with an__unstable-largevariant for the future 40px default.Todo
Testing Instructions
TBA...
npm run test-unit packages/components/src/slider/testScreenshots or screencast
Screen.Recording.2022-08-05.at.11.48.06.am.mp4