Double click block to exit zoom out mode #64573
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
getdave
added
[Type] Enhancement
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +78 B (0%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
|
I like this, and it makes sense to me. I don't see this happening by mistake easily. @richtabor I'd like your thoughts on this one. |
jeryj
reviewed
Aug 21, 2024
packages/block-editor/src/components/block-list/use-block-props/use-zoom-out-mode-exit.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-list/use-block-props/use-zoom-out-mode-exit.js
Outdated
Show resolved
Hide resolved
|
Flaky tests detected in 62707d0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10504914533
|
jeryj
reviewed
Aug 23, 2024
| event.preventDefault(); | ||
|
|
||
| __unstableSetEditorMode( 'edit' ); | ||
| selectBlock( clientId ); |
There was a problem hiding this comment.
This should already be selected due to the focus handler in block props selecting the block. Could we remove the selectBlock and leave a comment about where the block selection happens?
jeryj
reviewed
Sep 3, 2024
| const { __unstableSetEditorMode, selectBlock } = unlock( | ||
| useDispatch( blockEditorStore ) | ||
| ); | ||
|
|
There was a problem hiding this comment.
Zoom out mode is fairly rare right now. Rather than adding a double click to all blocks, could we early return here if we're not in zoom out? That way, we only add these listeners when it's useful?
|
@jeryj If you have time / inclination to make the changes you suggest please go ahead. Otherwise I'll pick it up during this week 🤞 |
jeryj
force-pushed
the
add/double-click-to-exit-zoom-out-mode
branch
from
972c690
to
5d89474
Compare
…steners Reducing the number of useSelect calls and event listeners is important for performance. Rather than have a useSelect to check zoom out mode and add event listeners to every block, let's do the useSelect once in the provider and pass it in to the hook, and only add the event listener if we're in zoom out mode.
The block is already being selected via the useFocusHandler hook, so no need to repeat it here.
|
Made some changes to reduce |
getdave
commented
Sep 4, 2024
| @@ -728,6 +730,7 @@ function BlockListBlockProvider( props ) { | |||
| themeSupportsLayout, | |||
| isTemporarilyEditingAsBlocks, | |||
| blockEditingMode, | |||
| editorMode, | |||
There was a problem hiding this comment.
As we're touching the deps, we should monitor the editor performance after merging this PR.
There was a problem hiding this comment.
I was remembering #56994 about combining block store subscriptions, and assumed it would be better to add it here rather than check on each block. Maybe the performance potential isn't worth it though. I do like the implementation though, as it makes the final result quite clean.
There was a problem hiding this comment.
That's a good point about combining store subscriptions. Let's try it "as is" and watch the performance charts post-merge.
There was a problem hiding this comment.
I did some smoke test with running performance in the inspector and entering into/out of zoom out mode and this method of using the combined subscription and not adding listeners was always faster in both total scripting and rendering, so I say we go with it as well.
There was a problem hiding this comment.
Great. I say we merge it and then watch the performance dashboard in case of any changes post-merge.
There was a problem hiding this comment.
@jeryj Notice any problems in the Dashboard? I'm not seeing any.
getdave
commented
Sep 4, 2024
packages/block-editor/src/components/block-list/use-block-props/use-zoom-out-mode-exit.js
Show resolved
Hide resolved
getdave
commented
Sep 4, 2024
|
@jeryj I can't approve my own PR but this feels pretty solid right now. |
jeryj
approved these changes
Sep 4, 2024
youknowriad
reviewed
Sep 10, 2024
|
|
||
| return useRefEffect( | ||
| ( node ) => { | ||
| if ( editorMode !== 'zoom-out' ) { |
There was a problem hiding this comment.
Instead of having the editorMode as a dependency of the useRefEffect and regenerating a new ref / new event listener everytime this change. We could just call getEditorMode within onDoubleClick event listener (Like we do for useNavModeExit
This would also remove the need to pass the editorMode top/down through the context...
There was a problem hiding this comment.
I'm seeing the opposite comment above. I think personally the performance impact of having the double click always there is less than re-rendering when the editorMode changes, it's also way simpler in terms of code. But I'm ok if you all disagree.
There was a problem hiding this comment.
I had the same thought process. I believe @jeryj did some performance profiling on this so he may be able to answer more concretely.
There was a problem hiding this comment.
I had done profiling on the page and not adding the event listener used a lot less resources. The metrics for typing are going up. I'm having a hard time isolating a PR that is causing it. It's odd to me that adding the mode to the block props would cause that, as the editing mode doesn't really change often and definitely isn't changed in the middle of the performance tests. I can make another branch that reworks this to this older style and we can compare.
There was a problem hiding this comment.
I don't see any major changes in performance with either approach.
There was a problem hiding this comment.
The typing metic was probably affected by a new store subscription. See #65337.
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?
Adds ability to exit Zoom Out mode by double clicking on the block.
Note: the accessibility implications of click handlers are discussed in
Testing Instructions for Keyboardbelow.Why?
It's important experiment with intuitive ways of exiting Zoom Out mode so that users can get back to edit mode.
How?
Adds a double click handler to the block props which will exit zoom out mode if a block is clicked and we are in zoom out mode.
Testing Instructions
Testing Instructions for Keyboard
This action cannot be mirrored by keyboards. Therefore we have introduced an alternative means of exiting zoom out mode in #64571.
Screenshots or screencast
Screen.Capture.on.2024-08-16.at.10-51-12.mp4