-
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
Fixed zoom out mode in edit template #65565
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @parthVataliya16. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @PARTHVATALIYA! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
Thanks for the contribution @PARTHVATALIYA ! I tested this out and the fix works. I can not speak to the code, but it addresses the issue. |
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.
Thanks for working on this. I find it strange that the notification component knows all this. Can we find a way to never have the notification if we're in zoom out mode?
packages/editor/src/components/visual-editor/edit-template-blocks-notification.js
Outdated
Show resolved
Hide resolved
Thanks @draganescu for reviewing the PR. I am new to gutenberg can you please explain me more. So I can resolve this. |
The EditTemplateBlocksNotification component should only show a notification, not also set editor modes and this PR adds too much to it. What I mean is that instead of editing the EditTemplateBlocksNotification component to not render if zoom out and to also set the editor mode, we need to find where the EditTemplateBlocksNotification component is rendered and render it there conditionally depending on editor mode. Also I suspect we won't have to reintercept double click since thre is something else already setting editor mode on double click, the bug to solve is to not show this notification. Not sure if I conveyed it right, is it any clearer? 😀 |
Thank you for explanation! Issue resolved. |
Thank you for working on this @PARTHVATALIYA! Apologies, I missed the ping earlier on this change and I submitted a very similar PR over in #65963. Are you keen to continue working on this one? If not, I'm happy to either jump in here, or continue on with the similar PR over in #65963. It seems the main next step is to look into the feedback from @draganescu regarding where we place this check, as arguably it'd be better if we didn't include the logic within the What do you think? |
Could we close this one out and then include @PARTHVATALIYA in props on your PR @andrewserong? |
@andrewserong you can continue with the PR #65963. |
Thanks for confirming @PARTHVATALIYA, and thanks for your work here, I'll be sure to include props for you over in #65963 when it lands! |
Closing in favour of #65963. |
What?
Part of - #65505
Why?
Solve - #65505
Testing Instructions
Screenshots or screencast
Pages.Template.gutenberg.Editor.WordPress.webm