Border panel: Update panel layout #35938
Merged
Conversation
This file contains hidden or 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: +92 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
andrewserong
approved these changes
Oct 26, 2021
Contributor
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
This is testing nicely for me @stacimc, with the PR working the same before and after rebasing on top of #35621. I tested in the post and site editors, the single column width panel items worked correctly, and the color control no longer shows the 'clear' button (but resetting values works well from the menu) 👍
I noticed in that other PR that the single-column class usage was removed from the Storybook examples to avoid adding the class name. The solution over there was to use styled( ToolsPanelItem ) to set the style instead of an additional class. Since it doesn't look like the block-editor package is using Emotion directly for styling anywhere yet (please correct me if I'm wrong!), I quite like the way it's done in this PR — including the class name, but also providing our own styling local to the border hook and the sidebar in the site editor. I'm not sure if it's preferred way to do it, though, so just CCing @aaronrobertshaw here for another opinion 🙂
Contributor
|
I haven't had the chance to look closely at this yet but at a glance, it mirrors the approach taken in #35911 for the Typography panel. When creating that PR, I couldn't see Emotion included in the block-editor package and I wasn't keen on introducing it only for these couple of panels. |
Contributor
|
Excellent, thanks for confirming, Aaron! This looks good to go to me, then 👍 |
Contributor
Author
I did use Emotion to create styled components at first, but backed the changes out for this exact reason 😄 I do want to call out that since we're using the ToolsPanel for border in the global styles sidebar, I had to use this selector: Which is not targeted to just the border panel. @aaronrobertshaw do you foresee any issue there? |
Contributor
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Thanks for all your effort on the border panels @stacimc 👍
✅ Border panels and support continue to work
✅ Border panel layout is consistent between block and site editors
I did notice a few very small rough edges to the color control within the ToolsPanel. Just things like multiple margins between the control's label and the selected color swatch or extra bottom margin on swatches causing increased gap before the next control.
These same issues are present with the PR switching the color block support to the ToolsPanel so not caused by anything in this PR.
I spent some time trying to find a means of applying some general styling for the color picker within the ToolsPanel context but was ultimately unsuccessful. The CircularOptionPicker component used implements a hardcoded class name and uses simple CSS styling itself. Given previous requirements of the ToolsPanel we can't introduce hardcoded classes into that component's styles thereby coupling the components too tightly.
In the end, I opted for adding styles specific to the colors support ToolsPanel. Perhaps an interim option, while still polishing the UI, could be to replicate those styles for the border panel? It would have the added downside of needing to be included again for the Global Styles sidebar though.
I'm open to better ideas to neaten up the color control.
| Before | After |
|---|---|
 |
 |
I do want to call out that since we're using the ToolsPanel for border in the global styles sidebar, I had to use this selector:
.edit-site-global-styles-sidebar .components-tools-panel-item.single-columnWhich is not targeted to just the border panel. @aaronrobertshaw do you foresee any issue there?
I don't see any issues with that for the time being.
The nature of the single-column class shouldn't go changing. Eventually, the idea is that the ToolsPanel could expose more in the way of the Grid component's props and either the ToolsPanelItem extends a GridItem component or expose extra props as well.
stacimc
force-pushed
the
update/border-panel-layout
branch
from
5fafc99 to
19be1c7
Compare
stacimc
requested review from
adamziel,
ajitbohra,
cameronvoell,
chrisvanpatten,
draganescu,
getdave,
guarani,
gziolo,
kevin940726,
mkaz,
mkevins,
mmtr,
nerrad,
noahtallen,
noisysocks,
ntwb,
oandregal,
spacedmonkey,
talldan and
tellthemachines
as code owners
stacimc
removed request for
TimothyBJacobs,
adamziel,
ajitbohra,
cameronvoell,
chrisvanpatten,
draganescu,
ellatrix,
getdave,
guarani,
gziolo,
kevin940726,
mkaz,
mkevins,
mmtr,
nerrad,
noahtallen,
noisysocks and
tellthemachines
Contributor
Author
|
Rebased to bring in styling changes to the color picker. Since this PR isn't open against trunk I inadvertently caused a bunch of auto-requests for reviews; apologies for the noise! |
stacimc
force-pushed
the
update/border-panel-layout
branch
from
19be1c7 to
3cdf0b0
Compare
This was referenced Nov 3, 2021
aaronrobertshaw
pushed a commit
that referenced
this pull request
Nov 17, 2021
* Restore single-column styling for appropriate controls * Remove clear button from Color picker in ToolsPanel
stacimc
added a commit
that referenced
this pull request
Nov 22, 2021
* Restore single-column styling for appropriate controls * Remove clear button from Color picker in ToolsPanel
stacimc
added a commit
that referenced
this pull request
Nov 24, 2021
* Restore single-column styling for appropriate controls * Remove clear button from Color picker in ToolsPanel
aaronrobertshaw
pushed a commit
that referenced
this pull request
Nov 29, 2021
* Restore single-column styling for appropriate controls * Remove clear button from Color picker in ToolsPanel
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.
Depends on:
Related:
Description
How has this been tested?
Clearbutton underneath the color controlScreenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).