Extract a BorderPanel component as a reusable component between Global Styles and Block Inspector #48636
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
…balStyles and block inspector
youknowriad
added
[Type] Code Quality
youknowriad
requested review from
noisysocks,
madhusudhand,
andrewserong and
aaronrobertshaw
youknowriad
commented
Mar 1, 2023
| return colorObject ? colorObject.color : colorValue; | ||
| }, | ||
| [ colors ] | ||
| ); |
There was a problem hiding this comment.
Note that I have to encode/decode colors every time in this component. Personally, I think that shouldn't happen in this component but instead it should be built-in in the lower level components BorderBoxControl... until ColorPalette because they all receive the "colors" presets as argument.
There was a problem hiding this comment.
Basically right now, if a user picks a "red" color from a palette, or picks a "red" color with the same value as a custom color, they are considered the same and automatically switched to "palette" colors. The problem is that the user intention might be different.
- If I pick a palette color, I expect the value to update as I update the palettes
- If I pick a custom color, I expect the value to be static.
This behavior is broken now and the value is always dynamic but this is how it works in trunk too, so I kept it as is.
youknowriad
commented
Mar 1, 2023
| return updatedSettings; | ||
| }, [ parentSettings, supportedStyles, supports ] ); | ||
| } | ||
|
|
||
| export function useColorsPerOrigin( settings ) { |
There was a problem hiding this comment.
This hook already exists but it's dependent on useSetting. I updated it here to have one that is independent of any context... Hopefully by the time we finish these refactors, we should be able to remove the other one.
There was a problem hiding this comment.
Actually I managed to use this in the other one to avoid duplication.
|
Size Change: +689 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 9c8a744. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4304257113
|
aaronrobertshaw
approved these changes
Mar 2, 2023
There was a problem hiding this comment.
Thank you for continuing the efforts to refactor all these panels @youknowriad 👍
The extracted border panel is testing well for me, I didn't spot any regressions comparing it against trunk's behaviour.
The one issue I encountered appeared to be a separate problem preventing global styles from being reset (fixed in #48648 I believe). After rebasing this branch locally it all worked smoothly.
For both Global Styles and the Block Inspector, I tested the following;
✅ Flat borders - single border value for all sides
✅ Flat border radii - single radius value for all corners
✅ Split borders - differing borders per side
✅ Consolidation of split borders - same value for each side gets reduced to flat value
✅ Split border radii - differing radius per corner
✅ Resetting individual controls e.g. border or border radius
✅ Resetting all border controls via reset all
✅ Configuring different default controls for the panel
Looks like you've already fixed the failing native e2e as well. Nice work 🚀
I'm happy to approve this as it LGTM but you're welcome to wait on a second set of eyes on the code changes also.
|
Thanks Aaron, let's get this one in and continue on other panels. |
3 tasks
|
@youknowriad it looks like there's a regression from this PR - #49594. Block border is always serialised now in the longer format that I mentioned in my comment. I had a brief chat to @aaronrobertshaw about it, and it sounds like global styles has always saved border in the longer format, but blocks previously preferred the short format when sides are linked. It had a look at a fix and considered adding a |
|
@talldan Thanks for the ping Dan. What about unifying the format between both? Can we use the long (or short) format in both and have the "engines" understand both for now (backward compatibility) |
|
Possibly. There was an explanation here about why global styles was always using the long format: gutenberg/packages/block-editor/src/components/global-styles/border-panel.js Lines 191 to 201 in 82ed8b7 @aaronrobertshaw knows more. In the table block, it might be possible to interpret |
|
That comment explains the gist of it. I'll try and expand more on it shortly but in a few minutes I'll have a potential fix up for the issue Dan raised and another small bug I found in this PR. The fix is working locally for me and follows Dan's suggestion of a |
|
Potential fix is up in #49603 |
I have a vague repressed memory of some issues with shorthand vs longhand styles and their specificity. Forcing global styles to split the border definitions avoided that at the cost of extra CSS. In general, however there was feedback on avoiding bloated CSS which combined with the need to still support the flat borders for BC lead to the current state. I'm out of time today but will happily dig further into this tomorrow, whether that is getting the full back story, testing alternate approaches e.g. unifying to longhand etc, or iterating on the current PR. |
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.
Related to #37064 and #47348 and similar to #47356 and #48070
What?
A year ago now, I've opened a discussion in #37064 that proposes unifying the components that are used in the block inspector and global styles. The idea is that we'd have components that look like this:
That component would render a panel used to edit a given style (say "typography" or "colors"), the
valuehas a well specified prop (that is mapped over the "style" object of a block or a global style "node" ). The idea is that we could use that exact same panel in both places, the only difference is a wrapper that map the "value" to the right place:Recently we realized this vision for the TypographyPanel and DimensionsPanel and the current PR does the exact same thing for the BorderPanel and uses the new UI component for both inspector controls and global styles.
To do
Testing Instructions