-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Button: Adopt support for border color, style, and width #44574
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
e0ff9c7
Button: Adopt remaining border support features
aaronrobertshaw 58e488c
Override core theme.json's button element style
aaronrobertshaw 47bfcc0
Fix typo
aaronrobertshaw 067f07f
Add additional detail to overrides comment
glendaviesnz 4ca13f0
Simplify comment
glendaviesnz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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.
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'm not a huge fan of having to add this much CSS to provide defaults that can overcome styles in core. If anyone has any ideas on a better approach, I'd be happy to evolve this one.
Without these tweaks the user has to set multiple fields before a border shows up. Even now the default border color is
currentColor
so when the background is white and the button text is white, the border isn't really visible.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.
It doesn't seem like we need to replicate this in
editor.scss
andstyles.scss
- if I removed this section from here the CSS fromstyles.scss
seemed to be applied in the editor and the frontend? I did a fresh build and a hard reload to make sure it wasn't just a case of the editor styles being cached.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.
We do need to add this CSS to overcome core's theme.json button element styles which reset
border-width
to0
.The theme.json styles get the
.editor-styles-wrapper
class prefixed to their selectors in the editor. As a result, those styles override the styles in the frontendstyle.scss
.Without these styles, a user needs to adjust the border width before they can see any border.
To witness this behaviour:
core/button
styles in your theme.jsonScreen.Recording.2023-02-07.at.3.13.43.pm.mp4
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.
Thanks for the extra detail - I was adding the border width and then the color so wasn't picking this up - if I had read back through the old comments I would have picked all that up - sorry for wasting your time here!
I have modified the comment to specify this detail like the one in style.scss does.