RangeControl: Improve initial hover interaction with Tooltip #20219
Conversation
This update improves the initial hover/mouseenter experience with the RangeControl's Tooltip. This is achieved be debouncing the interaction. By debouncing, the Tooltip rendering is less jarring when there are multiple `RangeControl` components next to each other. A Storybook story was added to test and simulate this experience. Lastly, the utility functions/hooks from `RangeControl` was abstracted to a dedicated `utils.js` file under the component directory.
ItsJonQ
requested review from
ajitbohra,
chrisvanpatten and
jorgefilipecosta
as code owners
| if ( timeoutRef.current ) { | ||
| window.clearTimeout( timeoutRef.current ); | ||
| } |
There was a problem hiding this comment.
Minor, but: window.clearTimeout will not complain if you give it undefined. Thus, the condition is not strictly necessary.
There was a problem hiding this comment.
Oh! Thanks for the heads up :). I'll make that update
Edit: I've noticed the clearTimeout guard in other parts of Gutenberg as well. Maybe something to adjust in the future (when we get around to it 😊 )
There was a problem hiding this comment.
I could imagine an argument on the basis of: Being explicit makes it clearer that there's an expectation that it might be unassigned. So even if not necessary, one might think it could help maintainability. For me, I think the benefits of simplifying it are slightly more (both in expressiveness, and smaller bundle size), so I'd lean more on the side of avoiding it.
...with other RangeControl hover interactions.
|
I don't know where best to report this, but I see some strange behavior using the range control in the Column block "Percentage width". Specifically, when the range knob is all the way to the left, sometimes clicking and dragging it will not move it. It's not consistent, but definitely disorienting when it happens. It seems like it's just not capturing the "drag" intention, because it otherwise just acts like default text selection when moving my mouse while mousedown. |
|
@aduth Ah! Thanks for reporting! @jorgefilipecosta reported similar issues that I'm working on resolving in this PR: Apologies for the 🐛 ! |
| * @param {number} value The value to clamp | ||
| * @param {number} min The minimum value | ||
| * @param {number} max The maxinum value | ||
| * @return {number} A (float) number |
There was a problem hiding this comment.
Two things:
- Each "column" of the docblock should be aligned, specifically the descriptions in this case.
@returnshould be separated by a newline (and is not subject to alignment with@paramparts)
* @param {number} value The value to clamp
* @param {number} min The minimum value
* @param {number} max The maxinum value
*
* @return {number} A (float) number
| const [ value, _setValue ] = useState( floatClamp( valueProp, min, max ) ); | ||
| const valueRef = useRef( value ); | ||
|
|
||
| const setValue = ( nextValue ) => { | ||
| _setValue( floatClamp( nextValue, min, max ) ); | ||
| }; |
There was a problem hiding this comment.
Personally, I don't usually care for _ prefixing, typically because there's usually a better way to distinguish whatever it is we're otherwise assuming to be "the same". In this case, one is responsible for directly setting the state value. The other sets the state value, but within particular constraints. Thus, I could imagine some naming like setValue and something like setClampedValue.
| @@ -25,6 +19,8 @@ import Button from '../button'; | |||
| import Dashicon from '../dashicon'; | |||
|
|
|||
| import { color } from '../utils/colors'; | |||
|
|
|||
There was a problem hiding this comment.
(We shouldn't want or need the extra lines amongst these imports, unless there's rhyme or reason to it)
|
@aduth Thank you for your feedback! I just pushed those changes <3 |
|
Size Change: +41 B (0%) Total Size: 861 kB
ℹ️ View Unchanged
|
Description
This update improves the initial hover/mouseenter experience with the RangeControl's Tooltip. This is achieved be debouncing the interaction. By debouncing, the Tooltip rendering is less jarring when there are multiple
RangeControlcomponents next to each other.A Storybook story was added to test and simulate this experience.
Lastly, the utility functions/hooks from
RangeControlwas abstracted to a dedicatedutils.jsfile under the component directory.How has this been tested?
Screenshots
Before
After
Checklist:
Follow up to:
#19916