Borders: Switch to ToolsPanel for displaying UI #33743
Conversation
aaronrobertshaw
added
[Type] Enhancement
|
Size Change: +385 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
aaronrobertshaw
force-pushed
the
update/border-support-to-use-new-panel
branch
from
c641f4b to
ad25333
Compare
aaronrobertshaw
force-pushed
the
update/border-support-to-use-new-panel
branch
from
ad25333 to
e0eb0d6
Compare
aaronrobertshaw
changed the title
|
I just took this for a test run. It works well for border supports in both the block and site editors 🎉
I know you're coming back to this after all the changes to the ToolsPanels, but as far as I could see there wasn't much more than updating the comments to refer to There' a WIP defaults PR for borders over at #34061 |
aaronrobertshaw
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
Thanks for reviewing this @ramonjd. Appreciate it 👍
Looks like I'd missed updating the border panel's CSS that referred to the In terms of the comments, I see references to a progressive discovery panel which is what the ToolsPanel is. The possibility of renaming the ToolsPanel has come up elsewhere so for the time being, maybe we should leave these comments. The nature of the panel won't change but the component's name might.
Thanks for catching the copy/paste error in the It should be |
There was a problem hiding this comment.
This is working well for me with the defaults in #34061 activated.
Independent of any blocking concerns/discussions, of which I'm not aware, this LGTM.
Thank you!
✅ I can apply and reset border support controls as expected. The corresponding inline styles are applied/removed.
✅ All changes are reflected on the frontend
✅ Updates to border styles in the Site Editor are applied to selected blocks
aaronrobertshaw
force-pushed
the
update/border-support-to-use-new-panel
branch
from
7333455 to
637f84f
Compare
aaronrobertshaw
changed the base branch from
trunk
to
add/virtual-bubbling-for-block-support-slots
|
Thank you everyone for testing the latest iteration of this! I appreciate it 🙇
Sounds like we're nearly ready to merge. @jasmussen or @jameskoster do you have any objections to us merging this PR and iterating on the border controls separately? @ramonjd already has a PR up to discuss and address which border controls will show by default for individual blocks. #34061 One final thing worth mentioning, is I think the PR @stacimc created, splitting out the multiple origin color palette update to the border color control, should be merged before this as it might make it easier to sneak that into 5.9 as a fix. |
|
To take stock. This is trunk: Border being something that should not be shown by default on any block (possibly with the exception of quotes in the future, if we manage to absorb the left border into theme.json), to me it then feels appropriate that this panel starts collapsed, but then also that it shows both width, style, and color at once when expanded. The big downside here is that you have to set both a width and a style for any border to show. This branch: Border being something you explicitly add using the "Plus" button feels good, and the fact that you can set a border width without also selecting a style is a key problem solved. The fundamental problem here is that width and style are two separate controls that both need to be added if you want to make a dashed line. While changing to dashed or dots isn't something I expect people to do that often, it still feels like those controls are separate only because In that vein, I still think that at least border-width and style (unsure about color) should be considered a single user interface control to add, rather than two separate ones. Can that happen in a followup? I think that depends mostly on whether we can come back to the panel and make it a single control in the future, without having to handle a slew of deprecations and related issues. If we can, seems fine to land. One thing I noticed though — which is possibly/probably unrelated to this PR, is that the panel when empty has the wrong height:
It's 53px tall when it should be 48: That height appears to be due to the component inheriting an arbitrary line-height from an ancestor. If we add
|
|
Thanks for the detailed comment @jasmussen 👍
It's not just border width you can set and immediately see a border. With this PR, you see a border regardless of which individual control you set, width, style or color.
In my testing across a few different themes, this isn't the case. You can select only border style from the border panel menu, select a style and see a border without the need to set a width. The current approach in this PR should pick up browser defaults.
I have no objections to this being a follow-up. I do not think it should block this PR given you can create a border via any of the individual controls and this is a improvement over the current state. If a mockup can be created for how a single control should look and function, I'll be happy to pick that up for implementation.
The border block supports (other than named colors) just add inline styles. Those inline styles are separate now and would continue to be so once combining controls together in the UI. There should be no deprecation issues as a result. This is the same as the The inline styles here would benefit from staying separate as per
You are correct this is unrelated to this PR. When the I'll fix this height issue in a separate PR. |
|
I added the tweak to fix the empty The plan now is to merge this Border panel PR first thing Monday. |
Just in case there was any doubt, the fact that style and width work as separate controls is commendable. But what I meant was that you should never have to add both style and width with two visits to the tools panel. There should be only a single "Border" control. Something like this:
I think there are still some border designs underway for improving how that looks in the future. So in the mean time and just for simplicity, we can use the current design:
☝️ That could be a single control called "Border" in the tools panel. You should never see only the width, or only the style. Does that sound okay?
Sounds good. The key is that it doesn't make a lot of sense to add only width and not also style, or only style and not also width. |
I could see a theme author wishing to disable border style to prevent dotted/dashed borders etc but still allowing variations in border width. I'll continue with the plan to merge this PR today and work on a separate PR to combine the width/style controls within the |
Creating the ToolsPanel for block supports around the slot resulted in the slot adding an extra div within the ToolsPanel breaking its layout. This div is needed for the slot's ref to be applied allowing virtual bubbling. This change effectively moves the ToolsPanel inside that. It also avoids needing to retrieve ToolsPanelContext and recreating the context provider.
This reverts commit 94f444c.
Update border support to display via progressive disclosure panel Updating border support UI styling for grid layout Update border global styles panel Fix border global style hasValue checks
These changes include adding a new border group to the inspector controls slots and updating the border tools panel to match the latest changes to that component.
* Restore single-column styling for appropriate controls * Remove clear button from Color picker in ToolsPanel
When a user first sets a border width or color their may not be a border style set. These changes aim to provide a fallback for that (in the frontend/block editor) or apply a border style by default in the site editor for Global Styles.
aaronrobertshaw
force-pushed
the
update/border-support-to-use-new-panel
branch
from
8956b49 to
3a8e748
Compare
| // can see a visible border. | ||
| const handleOnChangeWithStyle = ( setStyle ) => ( value ) => { | ||
| if ( !! value && ! borderStyle ) { | ||
| setBorderStyle( 'solid' ); |
There was a problem hiding this comment.
Do you happen to know why we do that here? This is creating some issues.
There was a problem hiding this comment.
There was feedback here that we needed to ensure when first setting a border width or color that there is a visible border. That involved ensuring a border style was set.
The inline comment above this line and the commit message point to that as the primary reason (3a8e748).
Depends on:
Dimensions Panel: Add new ToolsPanel component and update spacing supports #32392Related:
Description
This PR:
borderInspectorControls group slotToolsPanelToolsPanelDefault Controls
This PR only addresses the switch to the new
ToolsPanelcomponent. Deciding which controls should display by default for which blocks will be addressed via a separate PR.How has this been tested?
Manually.
Test instructions
Screenshots
Screen.Recording.2021-08-12.at.2.39.41.pm.mp4
Types of changes
New feature. Enhancement.
Checklist:
*.native.jsfiles for terms that need renaming or removal).