-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Close inserter on exiting Zoom Out to edit #65194
Conversation
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. |
It feels a little hacky having to rely on a block editor setting here. I would prefer if there was a means to subscribe to an action in another store and then dispatch an action. There might be one I'm not aware of... |
@getdave, batching shouldn't be required at the moment. The |
Size Change: +66 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
I think we should avoid a hasty abstraction here and just call the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this action is getting more stuff on its plate. Is this a correct approach? Should it dispatch something that closes the inserter and something that selects the correct block? Or, maybe, should we instead subscribe to SET_EDITOR_MODE
and react by closing the inserter and setting the proper selection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I would only want to close the inserter when double clicking or using the edit button. I don't think toggling the top level zoom out button in the header should also close the inserter. |
This comment was marked as outdated.
This comment was marked as outdated.
4ba19b4
to
42ba373
Compare
const { __experimentalSetIsInserterOpened } = getSettings(); | ||
|
||
if ( | ||
typeof __experimentalSetIsInserterOpened === 'function' | ||
) { | ||
__experimentalSetIsInserterOpened( false ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern follows the advice in #64573 (comment)
I've updated this PR to do this. LMK what you think @scruffian @jeryj. |
@draganescu It's not the right approach. I updated it to a simpler option which doesn't overburden the action. |
What?
Closes any open inserter when exiting Zoom Out when the user explicit opts to "edit" the content.
Closes #65161
Why?
See #65161.
How?
Call the relevant function provided on editor settings if available when the current mode is switching to non-zoom out.
Concerns
I'm not entirely happy with this approach for various reasons.
Do we want to always close the inserter when exiting Zoom Out or just in certain scenarios?Should we be batching the dispatch calls within__unstableSetEditorMode()
? (cc @Mamaduka)setEditorMode
?Testing Instructions
Edit
icon in the zoom out toolbarTesting Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2024-09-10.at.12-36-25.mp4