-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[RNMobile] Wrap mobile block toolbar Block Settings Button with ToolbarGroup #52715
Conversation
Size Change: 0 B Total Size: 1.43 MB ℹ️ View Unchanged
|
Flaky tests detected in f2d926c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5583836722
|
} } | ||
</BlockSettingsButton.Slot> | ||
|
||
<ToolbarGroup> |
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.
Nice that it was just wrapping the element! I was thinking if we should have it in the component instead? To be consistent with the other block controls, what do you think?
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 was thinking if we should have it in the component instead?
Yep, that's even better. I've updated the code. 👍
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've tested these changes using the demo app and they work as expected. 🚀
What?
Wraps the mobile block toolbar Block Settings Button with ToolbarGroup.
Why?
To match consistency with other design elements in the mobile block toolbar. ToolbarGroup provides consistent borders on the left and right of the Block Settings Button.
How?
Updates the code of
block-toolbar/index.native.js
to includeToolbarGroup
and wraps theBlockSettingsButton.Slot
component.Testing Instructions
Screenshots or screencast