Polish color indicator and itemgroup. #37028
Conversation
jasmussen
added
[Package] Components
|
@sarayourfriend I think you've touched this code at some point. I know you're busy elsewhere so this is just an FYI, don't feel like you have to respond. |
|
Size Change: +181 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
|
I like that this tried to improve the root component :). This one might be used though in unexpected places (I think the title of the |
| @@ -10,7 +10,7 @@ import { CONFIG, COLORS } from '../utils'; | |||
|
|
|||
| export const unstyledButton = css` | |||
| appearance: none; | |||
| border: 1px solid transparent; | |||
| border: 0; | |||
There was a problem hiding this comment.
I wonder why this was using a transparent border
There was a problem hiding this comment.
I wonder the same. It could be related to vertical height/metrics. In any case removing it allowed the border to go edge to edge instead of stopping in a triangular joint.
There was a problem hiding this comment.
This change currently breaks focus styles (see the &:focus rule a few lines below).
Leaving a transparent border allows us to only modify the border-color when the component receives focus, therefore avoiding an otherwise annoying "jump" that would be caused by adding a border
Let me know if I'm looiking in the right place, but gradients look good to me: When I investigated which component to edit, I found these:
|
There was a problem hiding this comment.
Hey @jasmussen , thank you for working on these changes!
For future changes to the @wordpress/components package, it would be better if you could open one PR per component (for example, in this PR we're introducing changes to both ColorIndicator and ItemGroup).
Regarding ColorIndicator in particular, some time ago we planned to explore creating a new ColorSwatch component, which would replace both ColorIndicator and CircularOptionPicker (more details in the ColorSwatch section of #35093), but unfortunately we currently lack the capacity to work on that.
When I investigated which component to edit, I found these:
We should check each instance of ColorIndicator and see if we can remove any style overrides (similarly to what we've done in this PR in packages/edit-site/src/components/global-styles/style.scss) or if we need to add any fixes after the changes in the component.
Finally, would you mind adding 2 CHANGELOG entries for ColorIndicator and ItemGroup ?
Thank you!
| & + & { | ||
| margin-left: 0.5rem; | ||
| } |
There was a problem hiding this comment.
This change could cause some layout glitches in other parts of Gutenberg
There was a problem hiding this comment.
I haven't found any places where this rule would apply. As is, I removed it because it appears to be dead CSS.
There was a problem hiding this comment.
I'm not sure it's dead CSS — this rule would potentially affect those situations where 2 or more ColorIndicators are rendered next to each other (at least in DOM order).
I scanned the codebase and found a few places that we should check to make sure we didn't introduce a layout regressions:
PanelColorGradientSettings- Global Styles'
Palette(although this may be already tackled by the changes topackages/edit-site/src/components/global-styles/style.scssin this same PR) - Global Styles'
Preview(although this may be OK because of theVStackelement wrapping around them)
| @@ -10,7 +10,7 @@ import { CONFIG, COLORS } from '../utils'; | |||
|
|
|||
| export const unstyledButton = css` | |||
| appearance: none; | |||
| border: 1px solid transparent; | |||
| border: 0; | |||
There was a problem hiding this comment.
This change currently breaks focus styles (see the &:focus rule a few lines below).
Leaving a transparent border allows us to only modify the border-color when the component receives focus, therefore avoiding an otherwise annoying "jump" that would be caused by adding a border
|
All good from the mobile editor side 👍 It uses its native variant for the component and styles. |
I'll revisit this piece and per your feedback to keep component PRs separate, probably restore that rule so this only touches the color indicator. Thank you! |
|
Thanks again for the feedback. I went ahead and reverted the ItemGroup changes (I'll revisit separately), and added a changelog entry for the colorindicator. Let me know if that looks good. |
|
This is where this PR falls short, we can update that panel title though to the new styles of the indicator though. |
|
Oh that's excellent, thanks for bringing my attention to that. I think I can polish that up! That part needed a little love anyway, don't you think? I think with the right positioning or margin, that little circle will be better. |
There was a problem hiding this comment.
Thanks again for the feedback. I went ahead and reverted the ItemGroup changes (I'll revisit separately), and added a changelog entry for the colorindicator. Let me know if that looks good.
Speaking of which, I think that there are a few changes introduced in this PR that can definitely cause layout regressions (as Riad already pointed out).
Apart from this conversation about the sibling selector, the fact that we're changing the height and we're removing the margin-left will definitely cause layout some layout regressions (as Riad already pointed out).
We should look for every instance of this component (I like to search for both the ColorIndicator and the component-color-indicator strings) and make the necessary tweaks, mostly:
- add a
margin-leftwhen needed - tweak vertical alignment and margin top/bottom (due to the change in height)
I know it can be quite a tedious task, apologies in advance! But I feel like these are definitely some good improvements — It's good that we're moving away from having a component that sets its own margin (which for me is an antipattern)
| & + & { | ||
| margin-left: 0.5rem; | ||
| } |
There was a problem hiding this comment.
I'm not sure it's dead CSS — this rule would potentially affect those situations where 2 or more ColorIndicators are rendered next to each other (at least in DOM order).
I scanned the codebase and found a few places that we should check to make sure we didn't introduce a layout regressions:
PanelColorGradientSettings- Global Styles'
Palette(although this may be already tackled by the changes topackages/edit-site/src/components/global-styles/style.scssin this same PR) - Global Styles'
Preview(although this may be OK because of theVStackelement wrapping around them)
jasmussen
force-pushed
the
polish/item-group
branch
from
bcab315
to
60090c3
Compare
|
Alright, I rebased and pushed tweaks based on your feedback. The global styles interface still looks like this: Now the dot in the collapsed panel looks like this: I ended up restoring the left margin, as it did appear to be used for the color indicator inside the collapsed panel. But the sibling selector still appears unnecessary. Here's the pertinent bit: Those margins should both be the same. So I just kept the simple margin and changed it to use the grid-units. One thing to consider is to not let the age of this component hold us back from cleaning things up. In my digging I found that it appears to have been used for color swatches in #7924. But those now appear to use the
The inline color picker looks like this now: That's the circular option picker component in use there as well. What do you think? |
|
I think this all makes sense but for me if we can, we should remove the margin as it's not the responsibility of a component to add margin to position itself. It can be used in a lot of variety of places and a margin is specific to one use-case and should move to that place instead. |
This sounds sensibile, thank you! I definitely trust your judgement when it comes to polishing the UI — if you're happy with the way it looks, then it's good for me too!
I'm not sure about
Absolutely! I hope my comments are not being seen as too much "holding back" — I am just trying to make sure that we don't introduce breaking changes and layout regressions. I would also like very much to, for example, remove those margins (I believe that a component should never set an outer margin on itself), but unfortunately these components have been around for a while and it's not easy to introduce breaking changes with the current state of Gutenberg and how packages are versioned / released. Edit: I agree with @youknowriad's comment — I think that the right solution would be to remove the margin rule from the As I mentioned before, we did have a roadmap for the changes that we wanted to apply to Color-related components, but unfortunately the components squad currently has very limited resources and no time to work on that. The fact that we decided to have the Global Styles sidebar in the upcoming WordPress release also shifted our focus on other lines of work, and therefore we didn't manage to make enough progress on this front. I'd love it if we could get some collaboration of moving the work on color components forward — and working on a new |
|
I think some small styles "breakage" is fine if we go into the right direction. That margin there is especially obvious to me that it shouldn't be part of the component and if there's a breakage because of that (outside Gutenberg core since we'll be fixing all of that), I think it's small enough to be ok. |
|
Thanks all, I'll take another look at the margin and see if I can fix the case where you have both text and background colors defined, and how that shows in the collapsed panel. That seemed to be the primary place that margin was used. |
|
Okay, try the last one, it uses gap instead of margin:
|
| @@ -7,6 +7,7 @@ | |||
| .block-editor-panel-color-gradient-settings { | |||
| .block-editor-panel-color-gradient-settings__panel-title { | |||
| display: flex; | |||
| gap: $grid-unit-15; | |||
|
The circles do look too small to me on that panel thing but I guess it's just taste :) |
|
Thanks both. I'll keep an eye on the php unit test update, rebase, and land this. Regarding the small color dots, we could make them stack horizontally like the GS presets. |
|
Ignore the php unit failure for now, it's a bug introduced in trac/core, they should be working on it. |
|
Alright, I've got passing unit test, PHP tests are failing temporarily. So I'll ignore those for now. Thanks again folks! |
Description
This PR tweaks some of the metrics of the color swatch indicator to match those of the Global Styles design (#34574), which look like this in trunk:
The circle is a bit big, 24px, should be 20, and each row in the itemgroup is a bit big. This PR polishes a bit to this:
20x20px circles, about 40px height itemgroup rows. Note also how the gray border at the bottom goes edge to edge. There used to be a barely perceptible diagonal in the corners:
This was due to a transparent 1px border, adding the joint in the corner.
Note that there's some padding math in the itemgroup component that appears there to adhere to a component size system, so I didn't touch it. This actually makes the itemgroup rows still e a bit too tall (42.38px rather than 40px). I'm curious if there's a better way to solve this:
Also note that this PR completely moves the color indicator styles from global styles into the color circle component itself. As far as I can tell that component is used in the native code, and in global styles. But in both cases it should arguably be identical. I'd appreciate if someone familiar with the RN stuff could give this a quick sanity check.
How has this been tested?
Test the color pickers in global styles.
Checklist:
*.native.jsfiles for terms that need renaming or removal).