RangeControl: Fix Reset and initialPosition #20247
Conversation
ItsJonQ
added
[Type] Bug
ItsJonQ
requested review from
ajitbohra,
chrisvanpatten and
youknowriad
as code owners
| .emotion-10 { | ||
| .emotion-18 { |
There was a problem hiding this comment.
Should we worry about how unstable these incrementing IDs are?
There was a problem hiding this comment.
Something to be observe I think 👍
To be honest, I've never used Storybook Snapshots together with Emotion prior to this project.
These updates snapshot changes make sense, as a push I made directly affect the styles that are computed/generated, which I suppose, affects the number of IDs that need to be incremented.
| @@ -48,6 +48,7 @@ const BaseRangeControl = forwardRef( | |||
| allowReset = false, | |||
| beforeIcon, | |||
| className, | |||
| currentInput, | |||
There was a problem hiding this comment.
I'm a bit confused. What is this prop? Why is it needed? How do we document it?
There was a problem hiding this comment.
To be honest... I'm not sure. But it was from the original RangeControl before the refactor 🙃 . I brought it back to preserve the value logic it had
There was a problem hiding this comment.
It's nor used or documented in Gutenberg so maybe we can pretend it was never there and forget about it?
|
Size Change: +4.49 kB (0%) Total Size: 866 kB
ℹ️ View Unchanged
|
|
Merged the latest! |
|
There is still one concern to discuss. When the tooltip shows up and I move the mouse up it prevents me from clicking the previous range control. Would it be possible to hide the tooltip as soon as you no longer hover the range control circle?
|
|
|
||
| useEffect( () => { | ||
| if ( valueRef.current !== valueProp ) { | ||
| setClampValue( valueProp ); | ||
| valueRef.current = valueProp; | ||
| } | ||
| }, [ valueProp, setClampValue ] ); | ||
| }, [ valueRef, valueProp, setValue ] ); |
There was a problem hiding this comment.
I can't immediately recall where the conversation, but I have some recollection of discussing that it may be a concern to include a useRef variable as a dependency of another hook?
There was a problem hiding this comment.
Ah, found it here: #20219 (comment)
So that hovering on a tooltip no longer keeps it open
|
Thanks for the reviews! Merging this up! |
* RangeControl: Fix reset and initialPosition logic * Add tests for desired initialPosition, value, and reset scenarios * Update storybook snapshots * Fix null value fill for marks * Remove unnecessary ref dependency in useEffect hook * Adjust Range x Tooltip interactions So that hovering on a tooltip no longer keeps it open
Description
This update is a followup to the recently updated
RangeControl.It resolves how values are outputted based on
initialPosition, external/internalvalue, and Reset handling.These are some scenarios this update resolves:
If there is no
value,initialPosition, orcurrentInput, the slider should appear to be centered, with nothing showing in the NumberInput.If there is an
initialPositionandvaluedefined, it should usevalue.If a reset action occurs, it will default to using
nullvalue. However, if the parent component uses an alternative value, it will resolve to that instead.How has this been tested?
Screenshots
The above GIF showcases the initial render with no defined values, resulting in a slider that's appears 50% filled. After adjusting the values, clicking reset will shift it back to 50%.
Types of changes
Takes care of some of the issues describe:
#20236 (comment)
Checklist:
Thank you! 🙏