Site Editor: Unify the delete button style in the dropdown menu with red #52597
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +5 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
t-hamano
added
[Focus] Accessibility (a11y)
|
Flaky tests detected in 323b05c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5543565555
|
Mamaduka
approved these changes
Jul 14, 2023
| <MenuItem | ||
| isDestructive | ||
| isTertiary |
There was a problem hiding this comment.
Do we need isTertiary here? The other menu items I checked don't have this prop, and it adds different background on hover.
There was a problem hiding this comment.
Do we need isTertiary here?
I think isTertialy prop is necessary. Because without this prop, the border will be drawn even if there is no focus.
isTertialy=false
tertialy_false.mp4
isTertiary=true
tertialy_true.mp4
I checked the code base again and noticed that there is only one Delete button that does not have the isTertiary prop. It's the page delete button. Therefore, I added the isTertiary prop to this button as well.
My feeling is that the default red border is unnecessary, what do you think?
There was a problem hiding this comment.
My feeling is that the default red border is unnecessary, what do you think?
I'm not a designer :) but I'd say I agree it's unnecessary. Thanks for working on this ❤️
There was a problem hiding this comment.
Marking for design feedback. The "tertiary" variation changes the background on hover, which doesn't happen with other buttons. It could be out of place.
There was a problem hiding this comment.
Thanks for the ping.
Neither of these seem quite right to me... The $theme color background when hovering destructive tertiary buttons looks a bit strange with the red text. But I also agree that the border that appears when the button is not tertiary seems heavy-handed and unnecessary.
I'd be inclined to update the latter. IE remove the box-shadow applied to .components-button.is-destructive:not(.is-primary):not(.is-secondary):not(.is-tertiary):not(.is-link). Then we can remove isTertiary here.
When a destructive button with an outline is required, isSecondary can handle that use case.
Side note: I've seen it suggested elsewhere to remove the red styling on destructive buttons altogether, as it is not a globally appropriate color. Still, getting everything into a consistent state from which we can later refine seems like a good change to me.
There was a problem hiding this comment.
remove the
box-shadowapplied to.components-button.is-destructive:not(.is-primary):not(.is-secondary):not(.is-tertiary):not(.is-link). Then we can removeisTertiaryhere.
This appears to be the ideal approach. Indeed, it is strange to have a border by default when in distractive state. However, should this be submitted as a separate PR for review by the component team?
There was a problem hiding this comment.
Yes, it would probably make sense to tackle that in a dedicated PR, then revisit this one.
There was a problem hiding this comment.
Thank you, then I would like to create a separate PR for the button component first.
| @@ -254,6 +254,8 @@ function GridItem( { categoryId, item, ...props } ) { | |||
| /> | |||
| { isCustomPattern && ( | |||
| <MenuItem | |||
| isDestructive={ ! hasThemeFile } | |||
| isTertiary={ ! hasThemeFile } | |||
tellthemachines
removed
the
Backport to WP 6.7 Beta/RC
|
Update: In #53607, the default border was removed from the destructive button. This PR also reflects the latest trunk and removes the Additionally, the "Move to Trash" button has had its default border removed as well.
|
|
Seems good to me! I've seen @jasmussen question whether red colorisation is appropriate for these actions so it'd be good to get his eyes on this. Still, the code change is relevant; if we want to change the treatment of |
|
Nothing blocking, since the "destructive" prop already exists, fine to move forward, and thanks for the ping. But the reason for reluctance there is that the color does nothing to help folks that are low-vision or suffering from color-blindness, so in many cases the color accent is at best meaningless, and at worst confusing: "oh wait, are there no revisions? Is there no undo?" And in the cases where something is truly destructive, we should always follow with a confirm dialog, whereas anything else can be basic. So as a larger effort, I'd personally revisit the destructive prop in the first place. |
I agree with you on this point. However, if this is considered a future improvement across the board, is it OK to merge this PR for now? |
|
Definitely no blockers from me, something non urgent to consider separately. |
This was referenced Oct 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #52510
What?
This PR unifies the delete button in the dropdown menu of the site editor according to the following rules:
Why?
It makes sense to apply the same style to actions that have the same meaning. I also think that applying red to destructive actions is consistent with WP-Admin style rules.
How?
In the Pattern Management dropdown menu where this PR is making changes, it will say "Delete" if the pattern was created by the user or "Clear customizations" if it was bundled with the theme. I did not consider the latter action to be destructive, since the content can be reverted from the revision.
Testing Instructions
Check the color of the following buttons:
Should be red
Should be the default color