-
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
Move Shadow controls to Border panel #58466
Conversation
Size Change: -15 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
Flaky tests detected in e4c942d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7798314771
|
829ae8a
to
e2aabd6
Compare
19521df
to
3081a05
Compare
packages/block-editor/src/components/global-styles/border-panel.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/global-styles/border-panel.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/global-styles/border-panel.js
Outdated
Show resolved
Hide resolved
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 Core SVNIf you're a Core Committer, use this list when committing to
GitHub Merge commitsIf 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. |
17381f7
to
77e9101
Compare
An aside, but we should not enable box shadow control by default. Does |
We can do that on each supported block, but I've also disabled it in the Border panel itself (14095e1), which could then be overridden — ie. set to I am wondering why, though! I would appreciate a reference on what to consider a default control in the future. |
It can be a default control in a sense that it's available, shadow is not something every site will use — having it front and center every time you click on a button is a bit much. It's the same reasons we don't have the line-height control visible all the time for Heading and Paragraph blocks. |
13cb245
to
b0e81f4
Compare
I think this is ready for another review. Screenshot@joen, @madhusudhand made an excellent point in that we were displaying the Border box controls directly under the "Box & Shadow" label, with no specification of what the control is for, so I've added a "Border" label only visible when Shadow controls are available:
I'm wondering if the label is fine as "Border" or if we should use something even more specific, like "Border box", which is what the component for those controls is called. |
b160ff0
to
b51e344
Compare
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.
Tested the changes in block and global styles. Pushed a small fix b51e344ab3ccb5fc274f0bbf104419e342661d32, also re-based with trunk.
LGTM. 👍
6612b21
to
e4c942d
Compare
@richtabor Similar to other controls, it is addressed and works in block styles, but not in global styles. (it is also same for other controls such as |
What?
Moves the Box shadow controls into the Border panel, in order to keep the "Effects" panel name free for possible future additions via the Interactivity API.
Screenshot