Spacing visualizer: add option to trigger with mousover as well as value change #44955
Conversation
glendaviesnz
added
the
[Feature] Design Tools
|
This approach involves a bit of prop drilling. I tried an approach using a Context provider, but that required the spacing controls to have knowledge of which parent control they belonged to which seemed like too tight a coupling ... but will explore some other approaches as well. |
|
Size Change: +130 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice improvement! 👌
It tests pretty well for me. There are a couple of small things I think we can tweak, which I've commented on inline.
Unrelated to this PR, I also noticed that the RangeControls aren't using the same step value as the UnitControl based on the unit. This seems a little clunky to me, is this something we can tweak separately or was this a deliberate choice?
Also, when there's no padding/margin value set, hovering still appears to almost add a border to the block, but the top of that was missing.
Test video
Screen.Recording.2022-10-14.at.11.23.57.am.mp4
Looks like the visualizer sets a default border, will add a check next week to remove this if size is |
|
It works really well so far, I think this change makes a big difference. Is the same effect for focus events also something you looked at or plan to look at? The thing that's noticeable with this change is that hovering left/right/bottom/top shows every side, and I think it could make the side that's being hovered more prominent (maybe fade out the other sides slightly). That could be a seperate PR though. |
|
This direction looks good to me. The current setup is also finicky with the useMemo call for the presets setup, so sometimes the overlay disappears way too quickly. Keeping it visible during the interaction (hover, dragging, engaged with dragging but not moving) makes more sense. |
@aaronrobertshaw I thought I saw this on Friday, but I can't replicate it today: 0-border.mp4do you still see this with the latest version of this PR? |
aaronrobertshaw
changed the title
There was a problem hiding this comment.
Thanks for the quick turnaround in updating this one 👍
Making the BoxControl trigger the visualizer feels much better. It looks like we might still need to update the CustomSelectControl to use the onMouseOver and onMouseOut handlers you pass to it. Hovering that form of the control doesn't display the visualizer for me.
We'll also need a components package changelog for the updates to the BoxControl & CustomSelectControl.
do you still see this with the latest version of this PR?
In the latest version of this PR, I do still notice the visualize border under the following conditions:
- Padding: When no padding value is set but not when it is set to
0 - Margin: When the margin value is set to
0
I don't think this issue needs to hold up this PR if you'd like to address it in a follow-up.
Latest testing video
Screen.Recording.2022-10-17.at.10.15.16.am.mp4
The thing that's noticeable with this change is that hovering left/right/bottom/top shows every side, and I think it could make the side that's being hovered more prominent (maybe fade out the other sides slightly). That could be a seperate PR though.
This would be another nice improvement but agree it could be a follow-up.
|
@aaronrobertshaw I think I have sorted the issues you noted. Currently, the mouseOver only works on the unexpanded select box. Once you expand the list to select an option it goes. It would be nice to have the size change as you scroll the list of options, but this involves wiring through a prop from the underlying downshift library which the components team weren't keen to do when it was discussed earlier as they were planning a rewrite to remove this library, not sure where that is at, but we can probably look at adding this support in a follow up PR. |
There was a problem hiding this comment.
Thanks for the continued iterations on this 👍
Once you expand the list to select an option it goes. It would be nice to have the size change as you scroll the list of options
Agreed, this would finish off the on mouse over highlighting nicely. Is it worth checking it with @ciampo et al. to see if this use case does move the needle at all in terms of making the required change? I'm also fine with it being a follow-up to keep this PR moving.
I think I have sorted the issues you noted.
The latest changes do fix the border issues I noted in my earlier review thanks.
While testing, I noticed some further odd behaviour. I couldn't "click and drag" the slider while making an initial selection. After that initial selection, it worked. It also appeared only to be a problem with the "linked" form of the control, i.e. after unlinking an unset control, I could click and drag the initial selection of the individual sides.
Demo of above issue
Screen.Recording.2022-10-18.at.5.13.49.pm.mp4
Other than that, I think some of the changes in the generation of styles for the margin visualizer could be cleaned up, along with a pair of typos in the changelog entries. I've dropped a few inline comments on those.
I could replicate this on trunk, so I will put in a separate issue for it tomorrow. |
There was a problem hiding this comment.
Agreed, this would finish off the on mouse over highlighting nicely. Is it worth checking it with @ciampo et al. to see if this use case does move the needle at all in terms of making the required change? I'm also fine with it being a follow-up to keep this PR moving.
I haven't gotten myself 100% familiar with what this PR does, but in general my preference would be to hold off on any changes to CustomSelectControl, as we're planning on refactoring it soon, likely removing the dependency from downshift.
This approach involves a bit of prop drilling.
There's quite a lot of prop drilling involved, although I'm also not sure if there's a better/easier way. I guess at most we could look into using the rest { ...props } syntax more? But that's not a game changer.
| @@ -131,6 +131,20 @@ Start opting into the unconstrained width style that will become the default in | |||
| - Required: No | |||
| - Default: `false` | |||
|
|
|||
| #### onMouseOver | |||
There was a problem hiding this comment.
I guess it makes sense to add these 2 functions to the documentation for now. In future refactors of the component, I guess we won't need to to do if we use the WordPressComponentProps type, since it inherits all standard HTML attributes/props.
There was a problem hiding this comment.
The continued refinements here look good @glendaviesnz.
Everything that hasn't been earmarked for a separate follow-up is working well for me. So from my end, I think this is good to go 🚢
A final thought that crossed my mind was whether adding border-radius to the padding visualizer styles my make its display a little neater for blocks also applying border-radius. Definitely not something for this PR but perhaps one to add to the list.
packages/block-editor/src/components/spacing-sizes-control/index.js
Outdated
Show resolved
Hide resolved
…izer" This reverts commit 5ad77fe.
glendaviesnz
force-pushed
the
add/mouseover-to-visualiser-2
branch
from
63b1a6b
to
f43ece5
Compare
|
It looks like we introduced a small regression to the dimensions panel's behaviour with the on hover visualizers. I've create an issue with further details here: #49124. |
What?
Adds a mouseover trigger to the padding/margin visualiser
Why?
Part of improvements to visualiser in #44700
How?
Passes down a mouseover handler to the spacing controls
Testing Instructions
Add a Group block and try setting margin and padding, and check that visualiser displays while moused over the spacing control
Screenshots or screencast
Before:
vis-before.mp4
After:
hover-permanent.mp4