Spacing: make visualiser appear on focus #46096
Merged
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
glendaviesnz
added
[Type] Enhancement
|
|
|
Size Change: +57 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
glendaviesnz
force-pushed
the
add/onfocus-to-spacing-visualiser
branch
from
1d48263 to
f1e3110
Compare
There was a problem hiding this comment.
Nice improvement @glendaviesnz 👍
This tests well for me when the spacing control is either the slider or the custom input field.
When there are enough spacing presets to trigger the control to use the CustomSelectControl, focusing it doesn't display the visualizer. Can we pass it the same onFocus and onBlur handlers?
Demo of CustomSelectControl
Screen.Recording.2022-11-28.at.9.55.15.am.mp4
It might also round out the experience if we can get the UnitControl's unit select to trigger or maintain the display of the visualizer.
Demo of Unit Select Focus
Screen.Recording.2022-11-28.at.9.51.20.am.mp4
As a side note, when testing the control's focus using the keyboard, I noticed that when the value is unset, we get an odd empty tooltip. Perhaps we can clean that up in a follow-up?
Screenshot of empty tooltip
Done, this control is being rewritten, but adding these now can help to inform the props needed on the new version maybe?
Done, required an update of the underlying component. @aaronrobertshaw let me know if you think these component changes are correct/above board and I will then update the components changelog
Yeh, I think we should do this in a followup, as it looks like the only way to do this currently would be to return early from the tooltip component if the value is undefined, and probably need to do some testing to make sure no backwards compat issues with this change. |
There was a problem hiding this comment.
Thanks for the quick iteration on this 👍
Done, this control is being rewritten, but adding these now can help to inform the props needed on the new version maybe?
While it is being rewritten, I think this is a valid need now, and your point regarding informing the new version's requirements is a good one. If we are to move forward with this we'll need to update the README.md as well.
let me know if you think these component changes are correct/above board and I will then update the components changelog
They do the job and look ok to me. I don't think there's any harm in getting the components squad gurus to chime in on this and the point above.
|
@aaronrobertshaw thanks for the follow-up reviews, I think everything you suggested is covered off now. |
Looks good to me 👍 You're right that in general we try to use |
This reverts commit 9a49313.
glendaviesnz
force-pushed
the
add/onfocus-to-spacing-visualiser
branch
from
ee6346d to
26d98ac
Compare
aaronrobertshaw
approved these changes
Nov 30, 2022
There was a problem hiding this comment.
I've given this another test, and it looks ready to land to me.
✅ No typing errors
✅ CustomSelectControl & UnitControl unit tests pass
✅ Components test ok in the Storybook
✅ Visualizer shows on focus for "all" slider
✅ Visualizer shows for custom select control
✅ Visualizer shows for custom value input, unit select, and slider
The only minor rough edges I spotted were:
- The empty tooltip when a control has an undefined value (to be addressed in follow-up)
2 The visualizer can, under some circumstances, exceed the viewport bounds triggering a horizontal scrollbar. (Very much an edge case and not a blocker)
mpkelly
pushed a commit
to mpkelly/gutenberg
that referenced
this pull request
Dec 7, 2022
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.
What?
Makes the spacing visualiser appear when the control is focused as well as on mouseover/mouseout
Why?
Currently the visualiser does not display when the control is focused, and so people tabbing between controls rather than using the mouse will not get the same experience as mouse users.
How?
Duplicates the mouseover/mouseout callbacks to onfocus/onblur
Finishes: #44700
Testing Instructions
Screenshots or screencast
focus.mp4