Block: Outline when interacting with Toolbar Block Type/Movers #20938
Conversation
ItsJonQ
added
[Type] Enhancement
ItsJonQ
changed the title
|
Size Change: +394 B (0%) Total Size: 857 kB
ℹ️ View Unchanged
|
|
This feels very good, thanks for putting it together. A few notes:
|
|
This is what I see:
The principle here feels very solid, and it is extra helpful when moving blocks:
I would echo Matías comments exactly, by the way. As you can see in the GIF above, the mover control also seems to collapse whenever a moving action is taken. The mover control should just remain visible whenever focus is inside. To Matías point about simplifying the states, it I wonder if you can take inspiration from his PR on adding a fade-effect between edit and select modes: #20933 — that very same fade would be really nice to have here as well! |
|
Thanks for the feedback all!
@mtias I can give that a try
I noticed this yesterday. Reason: When the move happens, something in the code is forcing focus from the It's tricky. I'll figure something out.
I believe we can simplify 💪
@jasmussen I can look into the fade effect! Thanks for the suggestion 🤗 |
Is this also the case in master? Awesome work! |
@jasmussen It is not (phew). I think the state updates to render the outline is making it unhappy. Will work on refining 💪 |
|
@jasmussen + @mtias I pushed up changes that improve the experience as suggested (above). If you folks would test it again, that would be lovely!
I took a look. Although these states render the same UI (borders around the block, rendered with
With that being said, I think keeping them separate is okay for now. |
|
This is what I see:
This feels REALLY solid. I was about to suggest that you could reduce those 250ms to 0 for fading the outline out, but I realize it's very nice that the outline fades out at the same time as the mover control collapses. Overall this feels like a substantial improvement purely on behavior, and I'd love for us to ship this as fast as we can. In that vein, the only remaining discussion is the matrix of states. I do wish we could collapse some of those. It seems like the focus state might be leveraged with the "isolation" interface that FSE might enter when editing a template part. I also wonder if we can merge is-selected with is-multi-selected, with the toolbar differentiation happening elsewhere. But this is sort of high level and separate. If there isn't realy a way to attach the highlight without this extra state class, I'm personally fine with it. Nice work Q! |
@jasmussen Thank you!!
Awesome! Is there anything you think we need to adjust?
Assuming all the various state
Where a new singular I think it would be out of scope for this PR. Just sharing for consideration :) |
|
I generally like the sound of that! Agreed probably out of scope for this one, but a good thought. |
| * | ||
| * @return {boolean} Whether the block is currently highlighted. | ||
| */ | ||
| export function getIsBlockHighlighted( state, clientId ) { |
There was a problem hiding this comment.
This should be named just isBlockHighlighted for consistency.
| * | ||
| * @return {string} Updated state. | ||
| */ | ||
| export function blockHighlighted( state = '', action ) { |
There was a problem hiding this comment.
This one read a bit odd to me — perhaps highlightedBlock is better?
|
@ItsJonQ actually I'm seeing one small thing — after moving a block, the highlighted state seems stuck. Initially it makes sense because focus moves to the movers, but if I focus on another element of the toolbar it still doesn't go away.
|
|
@mtias Awesome! Thank you for that feedback! I'll work on refining that as soon as I can 💪 |
ItsJonQ
force-pushed
the
try/toolbar-outline-block-on-focus
branch
from
81bfd4b
to
c1cecda
Compare
ItsJonQ
requested review from
ajitbohra,
chrisvanpatten and
mkaz
as code owners
|
Why is Travis so mad at these updates 😭 |
|
Couple quick questions:
|
|
@aduth Thank you for replying 🙏
I don't think so, as it only engages when interacting specifically with these 2 elements:
Yes. I believe it should.
Ah! If there's a way to do that (that you know of), that would be lovely. |
Ah, I guess then the question is more of a "should it", but I trust the judgment of y'all here.
I had looked briefly at the code. It may be pretty straight-forward with the contextual form of the block toolbar, I'm not quite as sure with the "top toolbar" variant of it (which is rendered in the toolbar, and not within the block itself). |
|
Specifically, there is a context object which is provided by a block so that descendent components can make use of certain pieces relevant to the block's own state: |
|
Thanks @mtias and @ItsJonQ . Where I may be getting caught up is that in many blocks, many of its toolbar buttons also apply to the block as a whole (list style, column vertical alignment, heading level, paragraph alignment). It wasn't previously clear to me that the mover and block type button were "linked" already, so at least I see the consistency in this approach. |
|
@aduth Thank you for your suggestions on the
I think you expressed it just fine 😊 |
| } | ||
| }, | ||
| [ showMovers ] | ||
| [ handleOnChange, showMovers ] |
There was a problem hiding this comment.
handleOnChange will change every render (it's created in scope as a new function). Are we really benefiting from memoizing the callback in that case?
Additionally, we include showMovers as a dependency (presumably because it's an implicit dependency via handleONChange, but not onChange, which is likewise an implicit dependency.
There was a problem hiding this comment.
Thanks for your feedback!
In this case, the state of showMovers works as a guard for both setShowMovers and onChange. We only want to make the state change if it's different.
Re: handleOnChange. Are you suggesting something like this?
const handleOnChange = useCallback(( nextIsFocused ) => {
setShowMovers( nextIsFocused );
onChange( nextIsFocused );
}, [ onChange ])And with this, I think we can remove it from being a dependency from the debounced Show/Hide functions
🤔
There was a problem hiding this comment.
Probably more the opposite, to instead remove useCallback from debouncedShowMovers. Over time, I've come to see useCallback as something of a code smell.
i.e.
function debouncedShowMovers( event ) {
if ( event ) {
event.stopPropagation();
}
const timeout = timeoutRef.current;
if ( timeout && clearTimeout ) {
clearTimeout( timeout );
}
if ( ! showMovers ) {
handleOnChange( true );
}
}Would you see a problem with this alternative? For what reason do we want useCallback ?
There was a problem hiding this comment.
In this case, the state of
showMoversworks as a guard for bothsetShowMoversandonChange. We only want to make the state change if it's different.
But it's only incidentally accounting for changes in onChange on account for the fact that handleOnChange is a new function each render, which counteracts any benefits we might otherwise get from using useCallback. In other words, debouncedShowMovers will also be a new reference on every render in the current implementation, and would therefore be effectively no different than if we don't use useCallback at all.
There was a problem hiding this comment.
Would you see a problem with this alternative? For what reason do we want useCallback ?
No problems 😊 . I was using useCallback as that was something I've seen in many places in our codebase, and it felt like a good idea to continue. But like you've suggested, it may be pre-optimizing, and perhaps causing more trouble and it's worth.
There was a problem hiding this comment.
The way I've understood it is that useCallback is useful if and only if there's an important reason to avoid changing references, and that in all other cases it's not only premature, but often yields worse performance.
Related: https://kentcdodds.com/blog/usememo-and-usecallback
| }, | ||
| [ isFocused ] | ||
| ); | ||
| timeoutRef.current = setTimeout( () => { |
There was a problem hiding this comment.
It existed previously, but: Is there any chance that this function is called twice before the callback is invoked? Because it could cause a problem where the timeoutRef.current is assigned to whichever setTimeout was scheduled last, and only the last one would be unscheduled in the useEffect unsubscribe callback (i.e. the first one would still trigger its callback even after the component unmounts, causing a warning to be logged if it calls to set state).
There was a problem hiding this comment.
Oh! Hmm... Perhaps we should clear the timeout before setting it in this debounce function.
| // Sequences state change to enable editor updates (e.g. cursor | ||
| // position) to render correctly. | ||
| requestAnimationFrame( () => { |
There was a problem hiding this comment.
I don't have any other suggestions, but this code gives me vibes that it could be fragile (race conditions).
(requestAnimationFrame and setTimeout are often misused to address issues where we should be acting as a side effect of some specific state transitions, and only be lucky circumstance can the same effect be achieved with scheduled callbacks)
There was a problem hiding this comment.
This was my only work around :(. I'm not comfortable with the fragile vibes either. There's something happening (elsewhere in the editor) that does something with focus (or perhaps caret position). Without rAF the interaction breaks.
I realized this after the E2E tests kept failing (earlier)
Open to suggestions! 🙏
There was a problem hiding this comment.
I personally wonder if we need this unmount behavior at all. It could be just a matter of including a reducer logic where the flag should be set to false when we start typing or when the block is unselected. (Unless I'm not understanding the purpose of this hook)
There was a problem hiding this comment.
I merged but if we find a better solution, let's follow-up on it.
Resolved by correctly handling the unmount cycle for useEffect to reset the highlight block state.
…-block-list__block
ItsJonQ
force-pushed
the
try/toolbar-outline-block-on-focus
branch
from
b5f7d33
to
4194cd2
Compare
|
Rebased with latest |
|
Thanks for getting this one in, it adds a very meaningful detail to the experience. |
|
Thank you all! 🙏 ! |
Description
This update enhances the new G2-based Toolbar interactions to provide a highlighted outline for a block when interacting with the Block Type/Mover actions.
How has this been tested?
Screenshots
The GIF (above) demonstrates the outline when engaging with the Block Type/Mover actions.
Interaction
The outline shows when:
The style of the highlight is the same as the block's navigation mode styles. This can be seen by pressing ESC when on a block.
Top Toolbar
The highlight feature still works for Top Toolbar (as well as smaller viewports)
Types of changes
blockHighlightedstate to track the highlighted block, responding to Block Toolbar interactionsblockHighlightedstateBlockToolbarblock-list/blockChecklist:
Resolves: #20875