Hide block toolbar when spacing visualizer is showing #45131
Conversation
…sualiser is showing
glendaviesnz
added
the
[Feature] Design Tools
glendaviesnz
requested review from
ellatrix,
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
|
|
@talldan the previous idea I mentioned of adding a |
|
Size Change: +7.65 kB (+1%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
| const { startShowingVisualizer, stopShowingVisualizer } = | ||
| useDispatch( blockEditorStore ); | ||
| const onMouseOver = () => { | ||
| startShowingVisualizer(); |
There was a problem hiding this comment.
This is an interesting problem to solve.
From the point of a public API, if a plugin calls startShowingVisualizer it has the effect of hiding the block toolbar, which I think would be unexpected. I see why it's like that though—startTyping also works the same way. It does leave open the possibility of triggering other effects when a user starts or stops typing.
I'd question if it's needed though, and I think code like this might cause more confusion (e.g. I can imagine plugins calling startTyping to hide the toolbar when the user isn't actually typing, just to get the desired result).
One option is that the actions/selectors/reducer can be more specific to what it does, e.g. hideBlockToolbar / showBlockToolbar.
That way there's the possibility of converging the isTyping code and the visualizer code to use the same system for hiding revealing the toolbar. Possibly deprecating isTyping along the way.
I think @ellatrix might be familiar with this code, so it'd be good to get a second opinion.
There was a problem hiding this comment.
yeh, as I was adding this I was thinking to myself "the naming isn't right", hideBlockToolbar / showBlockToolbar is much better from the external API point of view so have switched it to that.
|
@talldan because We could have the |
|
It looks like
Maybe |
Ah, yeh, same name, different source.
I guess then the problem might be that in cases where you want to hide the outline you may not always want to hide the toolbar and vice versa ? 🤔 |
It's a difficult choice 🤔 . I don't think The alternative is separate The main points for me are that it's undesirable having two APIs (
|
|
@talldan I have changed it to The difference in mouse actions between the current |
…ire the stopTyping action
|
@talldan if we stop propagation of the mouse event in the context of the dimensions control then the |
There was a problem hiding this comment.
This is a nice improvement, thanks for iterating on the code.
I had a few observations in testing, and they may or may not lead to small adjustments in how this works:
- When there's no padding or margin, the toolbar is still hidden, but there's no visualization, which doesn't feel quite right to me when using it. Not sure if this is something that's easy to fix.
- When changing a value and hovering off, the toolbar can reappear momentarily before the visualizer is hidden. I think that's because there's a timeout related to changing the value.
- Hiding the toolbar is possibly only needed for the margin visualizer, as the padding can't overlap the toolbar. I'm completely on the fence on this one, in some ways it's nice to have it work consistently and hide the toolbar in both cases. Not sure, what do you think?
I would consider these things as low priority though and they shouldn't block merge.
| const onMouseOver = ( e ) => { | ||
| e.stopPropagation(); | ||
| hideBlockInterface(); | ||
| setIsMouseOver( true ); | ||
| }; | ||
| const onMouseOut = ( e ) => { | ||
| e.stopPropagation(); | ||
| showBlockInterface(); | ||
| setIsMouseOver( false ); | ||
| }; |
There was a problem hiding this comment.
Are the stopPropagation calls still needed here?
There was a problem hiding this comment.
It is there to prevent these mouse events from triggering the stopTyping action in anticipation of us merging that with the showBlockInterface. I could remove them until we actually do that work, but thought it might be worth adding them to prevent this inadvertently breaking when we do get to it.
|
Thanks @talldan
I thought this would be easy but it opened a can of worms, partly related to the timeout mentioned below.
This is related to keeping it working for the Select list when a theme has more than 7 spacing values. I tried fixing this, but combined with the option of not showing 0 values above turned out to cause a rabbit hole of problems 😄 I am going to merge it as is so you can pick up those actions and will add some follow-up issues to deal with the above problems.
Yeh, I wondered that, will leave as is and see what wider feedback is, easy to pull out of padding if the consensus is that it is not needed. |
Sounds good, thanks! |
What?
Hides the selected block toolbar when the padding/margin visualiser is showing.
Why?
Part of the visualizer improvements from #44700
How?
Adds new store action/reducer/selector to indicate when the spacing visualizer is showing, and uses the selector to show/hide the toolbar
Testing Instructions
Screenshots or screencast
Before:
toolbar-before.mp4
After:
toolbar-after.mp4