PaletteEdit: Add appropriate size props to Buttons #66590
Conversation
mirka
added
[Type] Enhancement
| @@ -215,6 +215,7 @@ function Option< T extends PaletteElement >( { | |||
| <Item ref={ setPopoverAnchor } size="small"> | |||
| <HStack justify="flex-start"> | |||
| <Button | |||
| size="small" | |||
There was a problem hiding this comment.
I can't tell if there's any intentionality to the height of these rows. @WordPress/gutenberg-design Is the natural height resulting from this fix acceptable, or do we want to maintain a certain row height?
| Before | After |
|---|---|
 |
 |
There was a problem hiding this comment.
This predates my involvement so I can't comment with confidence on the intentionality. Aiming for 40px rows seems acceptable to me though.
This whole UI likely needs to be revisited and standardised at some point.
There was a problem hiding this comment.
Ok, let's leave it like this for now until we have time to revisit.
| __next40pxDefaultSize | ||
| className="components-palette-edit__menu-button" |
There was a problem hiding this comment.
The last menu item is visible when the canReset prop is enabled. It was also missing a CSS class that set the width to 100%.
| Before | After |
|---|---|
 |
 |
There was a problem hiding this comment.
Classname addition makes sense to resolve this.
I just wonder if these buttons make sense to be menu items instead of buttons?
Not something to block the PR of course.
There was a problem hiding this comment.
Right, it looks like this could be a DropdownMenu.
@ciampo Should we add any new dropdown instances using DropdownMenuV2, or should we still use v1 until it's a bit more stable?
There was a problem hiding this comment.
Let's keep v1 for now, but use this as a place where we can use v2 soon (probably as soon as #66383 is merged)
|
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. |
| __next40pxDefaultSize | ||
| className="components-palette-edit__menu-button" |
There was a problem hiding this comment.
Classname addition makes sense to resolve this.
I just wonder if these buttons make sense to be menu items instead of buttons?
Not something to block the PR of course.
|
Flaky tests detected in 0661a62. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11596713065
|
* PaletteEdit: Add appropriate size props to Buttons * Add changelog Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
In preparation for #65751
What?
Adds appropriate size props to the remaining Buttons in PaletteEdit.
Testing Instructions
See the PaletteEdit story in Storybook.