-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
#30930 - Make it easier to select "Edit visually" when in "Edit as HTML #41516
#30930 - Make it easier to select "Edit visually" when in "Edit as HTML #41516
Conversation
…dit as HTML" mode
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @adambasa-dp! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
packages/block-editor/src/components/block-settings-menu/block-visually-convert-button.js
Outdated
Show resolved
Hide resolved
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.
Thank you for contributing!
The react native test failures seem unrelated, I just restarted them. Other than what @ZebulanStanphill noticed, this PR looks great to me. Let's update that name and get it merged!
packages/block-editor/src/components/block-settings-menu/block-edit-visually-button.js
Outdated
Show resolved
Hide resolved
The failed checks looks like flaky tests to me, I just restarted them |
@@ -20,6 +21,7 @@ export function BlockSettingsMenu( { clientIds, ...props } ) { | |||
/> | |||
) } | |||
</ToolbarItem> | |||
<BlockEditVisuallyButton clientIds={ clientIds } { ...props } /> |
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.
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.
@ZebulanStanphill I did it according to the design from that issue #30930
IMHO it's ok, but we can change the order
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.
I'd also suggest putting the button before the ellipsis (with a divider between) to avoid breaking a widespread UI convention.
Code LGTM. I see @carolinan requested design feedback, let's wait for it before merging. |
OK, I've just changed order of those buttons |
It works well, but I think a divider between the button and the ellipsis would help make this toolbar consistent with all others in the editor canvas. |
@jameskoster sure, I've just added a divider |
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.
const { isBlockMultiSelected, getBlockMode, getBlock } = select( | ||
blockEditorStore | ||
); | ||
const isSingleSelected = ! isBlockMultiSelected( | ||
firstBlockClientId | ||
); |
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.
Current unit test failure is caused by the recent update to wp-prettier
in trunk
. (See #40542.)
Here's what the reformated code should look like:
const { isBlockMultiSelected, getBlockMode, getBlock } = select( | |
blockEditorStore | |
); | |
const isSingleSelected = ! isBlockMultiSelected( | |
firstBlockClientId | |
); | |
const { isBlockMultiSelected, getBlockMode, getBlock } = | |
select( blockEditorStore ); | |
const isSingleSelected = | |
! isBlockMultiSelected( firstBlockClientId ); |
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.
Since that's the last required change and it's just code formatting, I went ahead and clicked "commit"
…-edit-visually-button.js Co-authored-by: Zebulan Stanphill <zebulanstanphill@protonmail.com>
🎉 Congratulations on your first merged PR @adambasa-dp ! |
Congratulations on your first merged pull request, @adambasa-dp! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts: https://profiles.wordpress.org/me/profile/edit/ And if you don't have a WordPress.org account, you can create one on this page: https://login.wordpress.org/register Kudos! |
What?
Make it easier to select "Edit visually" when in "Edit as HTML" mode #30930
Why?
#30930
@adamziel @carolinan