Button: Remove default border from the destructive button #53607
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: -8 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
t-hamano
commented
Aug 13, 2023
Comment on lines
-54
to
+58
| <MenuItem onClick={ () => removePage() } isDestructive> | ||
| <MenuItem | ||
| onClick={ () => removePage() } | ||
| isDestructive | ||
| variant="secondary" | ||
| > |
There was a problem hiding this comment.
Since this button has no variations, a default border is expected. Therefore, the secondary variation has been added. However, this border will be removed in the future in order to unify the style of the delete button in #52597.
t-hamano
commented
Aug 13, 2023
| <MenuItem | ||
| isDestructive | ||
| isTertiary | ||
| onClick={ () => setIsModalOpen( true ) } | ||
| > | ||
| <MenuItem isDestructive onClick={ () => setIsModalOpen( true ) }> |
There was a problem hiding this comment.
This button had the tertiary variation, but was removed as it is no longer needed.
t-hamano
changed the title
t-hamano
added
[Type] Enhancement
|
Thanks for the review, @mirka! |
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.
This PR was created based on the discussion in this comment.
What?
This PR removes the default border from the destructive button.
Why?
There are several delete buttons that combine destructive and tertiary statuses, mainly in the Site Editor drop-down menus, in order to unify the style with the default buttons without borders.
However, an unintended tertiary variation background color is applied on mouseover, even though we should only expect the default border to be removed.
If we want to apply a default border to the destructive button, we believe that a default border is unnecessary, because it can be achieved by combining secondary status.
How?
I removed the default box shadow. At the same time, I adjusted the destructive buttons in the codebase to maintain their current behavior.
StoryBook
"variant" variation
The button should display correctly for any variant property.
variant.mp4
"isBusy" variation
The button should display correctly for any variant property.
is-busy.mp4
"isPresed" variation
We can confirm that the black background color and red text color are unnatural and that some actions make the text invisible. However, since this also occurs in trunk, I believe it is outside the scope of this PR.
is-pressed.mp4
Block Editor
I will comment inline after this for buttons with changes made. For the following destructive buttons, removing the default border does not affect their behavior.
"Move to trash" button in the post editor
Since this button already has a secondary variation, the default border will still be displayed.
"Remove track" button in the tracks editor of the video block
The border is not shown from the beginning because it has a link variation.
"Delete menu" button in the navigation block
Since this button already has a secondary variation, the default border will still be displayed.