Border panel: Collapse color controls #37425
Merged
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
stacimc
added
the
[Feature] Design Tools
|
Size Change: +145 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
The border controls are being reimagined entirely in #35602. In the meantime, this PR greatly reduces the size of the panel by collapsing the colors. |
|
I tested in the post and site editors. ✅ Click on the Border color control and verify the popover opens. Test making selections.
✅ Verify that the Background, Text, and Link color options in the Colors panel continue to work.
This is not an issue with this PR, but I noticed that the floating panel isn't quite optimized for narrow screens: |
ramonjd
approved these changes
Dec 15, 2021
|
Thanks for working on this. I'm curious, the ItemGroup for the Paragraph has 2px rounded corners for the first and last items:
This one has square corners: Note that all of these itemgroups are too tall, 43px in global styles, 46px in the post editor. This is a separate issue unrelated to this PR. |
|
CC: @ciampo if you have any inputs on the rounded corners. |
@jasmussen Well spotted, thank you! The border in the Color panel is applied via the ItemGroup as you mentioned; I pulled out the single item Dropdown for use in the Border panel. I've updated this to refactor out the entire
|
stacimc
force-pushed
the
update/border-panel-to-use-collapsed-colors
branch
from
17277a8
to
f6cedd8
Compare
andrewserong
added
[Type] Enhancement
andrewserong
approved these changes
Dec 17, 2021
There was a problem hiding this comment.
Great work @stacimc, nice refactor! Just confirming this is testing well for me too:
✅ Panel matches the screenshots in the PR description
✅ Popover panel and selecting from custom or theme/default colors works well
✅ Border color is correctly reset via the ToolsPanel menu buttons
✅ Verified that the Background, Text, and Link color options in the Colors panel continue to work, and in the process made some fairly hilarious designs 😅
The only issue I ran into was the same as Ramon mentions about the popover position on mobile. That issue exists on trunk anyway, so this looks good to go to me, and we can always do follow-ups.
Exports the entire ItemGroup so that other panels that want to use the component without the wrapping PanelBody can still support multiple color items, and do not need to manually apply border styles. It continues to work when passed only a single item.
stacimc
force-pushed
the
update/border-panel-to-use-collapsed-colors
branch
from
f6cedd8
to
bacd29f
Compare
|
Rebased to fix merge conflicts. I'm seeing scrollbars on the popover now when the screen height is small, but confirmed this is acknowledged behavior and happens on trunk. |
ciampo
reviewed
Dec 18, 2021
Comment on lines
+40
to
+53
| <ItemGroup | ||
| isBordered | ||
| isSeparated | ||
| className="block-editor-panel-color-gradient-settings__item-group" | ||
| > | ||
| { settings.map( ( setting, index ) => ( | ||
| <Dropdown | ||
| key={ index } | ||
| position={ dropdownPosition } | ||
| className="block-editor-panel-color-gradient-settings__dropdown" | ||
| contentClassName="block-editor-panel-color-gradient-settings__dropdown-content" | ||
| renderToggle={ ( { isOpen, onToggle } ) => { | ||
| return ( | ||
| <Item |
There was a problem hiding this comment.
I don't remember if this exact situation already came up in another PR (cc @jasmussen ), but have you tried to have the ItemGroup inside Dropdown instead? ie.
<Dropdown>
<ItemGroup>
<Item>
There was a problem hiding this comment.
We did try that, yes, and there were ome problems with it. I can't recall them exactly, they are noted somewhere on one of Jorges PRs, and it's something to follow up on, but was acceptable for now. Thanks for looking, and thanks Staci!
|
Cherry-picked for 5.9.1. |
Mamaduka
removed
the
Backport to WP Minor Release
Mamaduka
pushed a commit
that referenced
this pull request
Feb 16, 2022
* Refactor out Dropdown component to be used by border panel * Update label text * Add border-radius * Refactor out the entire ItemGroup instead of just the Dropdown Exports the entire ItemGroup so that other panels that want to use the component without the wrapping PanelBody can still support multiple color items, and do not need to manually apply border styles. It continues to work when passed only a single item.
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.
Description
In #37067 the Color panel was updated to collapse the color elements for a cleaner looking/more compact set of controls. This PR updates the Border panel to do the same for the border color.
To accomplish this, it refactors out the
ColorGradientSettingsDropdowninto its own component so that it can be used by the Border panel.How has this been tested?
I used the Group block to test, since it supports Color and Border with all its properties.
Screenshots
With the popover open:
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).