Update: PanelColorGradientSettings to use dropdowns #37067
Conversation
jorgefilipecosta
added
[Type] Enhancement
|
Size Change: +583 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
Very nice. This feels a lot more natural than the drill-down option explored earlier. Its also easier to use when editing a few colors back-to-back; There's no need to find a back button, or wait for transitions. -- I noticed the Solid/Gradient toggle seems broken when first opening the color menu: -- I think we should likely look at a few more tweaks:
|
|
|
||
| .block-editor-panel-color-gradient-settings__item { | ||
| // @todo: this can be removed when https://github.com/WordPress/gutenberg/pull/37028 lands. | ||
| .component-color-indicator { |
There was a problem hiding this comment.
37028 has been merged, so these rules can be removed. That gets us to this point at least:
The diagonal line is missing. I'm realizing that this diagonal is scoped to work only in global styles: https://github.com/WordPress/gutenberg/pull/36994/files#diff-448ace8cb198438b19f4fc13f3107589730cf0aea599c46fa935e87ff1b70816R54
Where's a good place to move that rule? The diagonal should be shown when a color is unset, and the transparent checkerboard can be shown when a color is explicitly transparent. Any thoughts?
There was a problem hiding this comment.
The vertical alignment of the circles also seems off in the above capture
There was a problem hiding this comment.
A fix for that was merged in #37028, I wonder if a rebase would fix it?
There was a problem hiding this comment.
(Ignore this comment in case we're deleting this rule)
We should try to avoid using internal classnames for styling @wordpress/components (same as comment above)
There was a problem hiding this comment.
We need this rule because of the empty line but the class enhancement was done.
jorgefilipecosta
force-pushed
the
update/panel-color-gradient-settings-to-use-dropdowns
branch
from
a513207
to
a1c9467
Compare
|
You are all too fast and had me jumping from one PR to the other 😄 Good notes from @shaunandrews above, some of which I list out below as well.
I'm personally also ok compromising some of the above in the name of 5.9, and iterate further later, but there's likely some challenging a11y aspects switching between these panels. One temporary alt solution I don't love is to add a header to the pop over to close and drag. LMK if the above resonates with you or if anything further context is needed. |
I briefly thought about this, but then considered we wouldn't be using a "menu" anymore. I think that might have more drastic implications to usability and accessibility. I'm not arguing against it, just that it would likely mean a different implementation, maybe? |
There was a problem hiding this comment.
I’m assuming we’re mostly talking about things in SC 2.4.8 Location in regards to the a11y implications of these double popovers. (As opposed to the things we already support, e.g. focus management and keyboard operability.)
I agree it feels pretty reasonable when the popover content is minimal, but in other cases we may be pushing it. For example in this mobile viewport with Gradient options, we are basically looking at three layers of popover with an obstructed tab panel in between:
That’s a pretty deep stack to keep in your head, though it could be somewhat alleviated by things like #35292, or positioning the popovers in a way that makes the origins clearer. At least in the mobile use case though, it could be argued that the navigator approach in #36952 (or anything that reduces the popover stack) makes more sense. (I can't actually try it because the branch is already deleted.)
Maybe relevant is something like the Figma color flyout, which is designed in a way that doesn't require a secondary popover.
CleanShot.2021-12-03.at.04.11.55.mp4
| { children } | ||
| <ItemGroup isBordered isSeparated> | ||
| { settings.map( ( setting, index ) => ( | ||
| <Dropdown |
There was a problem hiding this comment.
It looks like the addition of the Dropdown wrapper here adds an extra element around each item inside the itemgroup. So without the dropdown you have this markup:
With the extra wrapper, we have this:
In terms of tabbing that doesn't seem to be a problem due to the tabindex properties. But the extra wrapper does mean this rule now is causing trouble:
The rule is around the use of border for focus inside the itemgroup, making it transparent if it's the last child in an itemgroup. But because of the extra container, it's now adding transparent borders to every dropdown container which makes them all 1px taller. Compare this context:
... with that of global styles:
My instinct tells me the solution is to refactor how focus works inside the itemgroup, to not rely on those advanced selectors, but perhaps instead just use a pseudo element. That would also let us avoid the use of transparent borders, which I think are problematic anyway due to them being made opaque in high contrast modes.
But before I explore a fix for this in a separate PR, I wonder: @ciampo do you think my instinct above makes sense, or would there be a different or better way to fix this?
There was a problem hiding this comment.
@jorgefilipecosta it looks like the ItemGroup component is designed to have a very specific structure of ItemGroup > Item (like ul > li) and that the border styles are designed for the DOM structure to be pretty specific. As noted above the addition of the Dropdown is causing a few small issues since the structure becomes ItemGroup > Dropdown > Item, making the border rules target the wrong element.
It's not problematic enough to block this PR, I think, but it's also not ideal.
Just to see if it was possible, I tried to see if it would work to move the Dropdown inside the Item. Like so:
<ItemGroup isBordered isSeparated>
{ settings.map( ( setting, index ) => (
<Item
key={ index }
className="block-editor-panel-color-gradient-settings__item"
>
<Dropdown
contentClassName="block-editor-panel-color-gradient-settings__dropdown"
renderToggle={ ( { onToggle } ) => {
return (
<HStack
justify="flex-start"
onClick={ onToggle }
>
<FlexItem>
<ColorIndicator
colorValue={
setting.gradientValue ??
setting.colorValue
}
/>
</FlexItem>
<FlexItem>{ setting.label }</FlexItem>
</HStack>
);
} }
renderContent={ () => (
<ColorGradientControl
showTitle={ false }
{ ...{
colors,
gradients,
disableCustomColors,
disableCustomGradients,
__experimentalHasMultipleOrigins,
__experimentalIsRenderedInSidebar,
enableAlpha,
...setting,
} }
/>
) }
/>
</Item>
) ) }
</ItemGroup>
It doesn't totally work, it replaces the UnstyledButton with the Dropdown. But is there a way we could make it work? Any thoughts?
There was a problem hiding this comment.
@jasmussen analysis is correct.
I've tried playing around with the markup a little, but I couldn't find any good solution — mainly trying to change the outline to something like:
ItemGroup
Item
Dropdown
Button
[content]
But that would mean that we'd need to apply most of the styles of <Item> to <Button> to achieve the same look&feel (and even then there could be some glitches)
I think that the best solution for now is to keep things as they are, and look into improving how ItemGroup and Item are styled directly at the source level.
jasmussen
force-pushed
the
update/panel-color-gradient-settings-to-use-dropdowns
branch
from
a1c9467
to
a5951b4
Compare
|
I took the liberty of rebasing this branch, and pushing a small fix to add the diagonal indicator for unset colors: Per the comment here, I'll look separately at how/if we can fix the spacing issue separately in the itemgroup. |
There was a problem hiding this comment.
I agree it feels pretty reasonable when the popover content is minimal, but in other cases we may be pushing it. For example in this mobile viewport with Gradient options, we are basically looking at three layers of popover with an obstructed tab panel in between:
I tend to agree with the points made by @mirka here as well
| .components-dropdown { | ||
| display: block; | ||
| } | ||
|
|
||
| // Allow horizontal overflow so the size-increasing color indicators don't cause a scrollbar. | ||
| .components-navigator-screen { | ||
| overflow-x: visible; | ||
| } |
There was a problem hiding this comment.
We should not rely on internal components classnames, as they are not part of the public APIs and could change in the future.
A better approach would be to set a custom className in the specific context of the block editor's color panel, and then use that new custom classname to apply styles (both for dropdown and navigator screen)
|
|
||
| // Allow horizontal overflow so the size-increasing color indicators don't cause a scrollbar. | ||
| .components-navigator-screen { | ||
| overflow-x: visible; |
There was a problem hiding this comment.
As @jasmussen mentioned here, we should be able to remove this rule safely
|
|
||
| .block-editor-panel-color-gradient-settings__item { | ||
| // @todo: this can be removed when https://github.com/WordPress/gutenberg/pull/37028 lands. | ||
| .component-color-indicator { |
There was a problem hiding this comment.
(Ignore this comment in case we're deleting this rule)
We should try to avoid using internal classnames for styling @wordpress/components (same as comment above)
| { children } | ||
| <ItemGroup isBordered isSeparated> | ||
| { settings.map( ( setting, index ) => ( | ||
| <Dropdown |
There was a problem hiding this comment.
@jasmussen analysis is correct.
I've tried playing around with the markup a little, but I couldn't find any good solution — mainly trying to change the outline to something like:
ItemGroup
Item
Dropdown
Button
[content]
But that would mean that we'd need to apply most of the styles of <Item> to <Button> to achieve the same look&feel (and even then there could be some glitches)
I think that the best solution for now is to keep things as they are, and look into improving how ItemGroup and Item are styled directly at the source level.
jorgefilipecosta
added
the
[Status] In Progress
|
Thank you all for the feedback it seems the technical changes are ok. I'm working on the design taking into account the feedback provided. For planning purposes, Is this change supposed to be part of WP 5.9 or it is something for the future? |
|
I think what's in this branch is a vast improvement over what's shipping in trunk, and I'd personally love to see it. |
youknowriad
added
the
Backport to WP 6.7 Beta/RC
|
This is a welcome UI enhancement.
|
jorgefilipecosta
force-pushed
the
update/panel-color-gradient-settings-to-use-dropdowns
branch
from
eddf8d7
to
9201bca
Compare
|
Thank you all for the feedback! I tried to incorporate the design feedback.
I faced some challenges because of underlying issues on the ToggleGroupControl. I added "temporary solutions" in this PR so we can test things properly. All the changes related to this component will be extracted into separate PR's so we can discuss the changes in a more detailed and focused way. Please ignore changes related to this component from this PR. Another issue is the custom gradient picker color popover not having a great sidebar behavior we will get that for "free" after #37115 is merged so also please ignore that until the other PR is merged. |
|
Took this for a spin, and high level this feels like a very big enhancement over what's shipping, where it's hard to figure out which color you're changing simply due to the multiple duplicated color palettes stacked.
I think there's going to be room to improve this further. For example the "Clear" button remains a bit of an odd one, but this can be enhanced by the color panel becoming a tools panel. I don't think we should remove the border around the popover, though. That full-bleed look works for the innermost color picker, and it might work for making the top swatch have that edge to edge look. But I think we should explore that separately, and keep the gray border in the mean time. That means this mockup from #35093 is probably the closest thing to target at this point. |
jorgefilipecosta
force-pushed
the
update/panel-color-gradient-settings-to-use-dropdowns
branch
3 times, most recently
from
9cb7fb6
to
2a58c4a
Compare
|
Hi @jasmussen, @ciampo, @mirka thank you for the reviews.
On mobile, it is not ideal, on the next iteration, it is probably something we should work on.
The suggestions regarding classes were applied. |
jorgefilipecosta
force-pushed
the
update/panel-color-gradient-settings-to-use-dropdowns
branch
from
2a58c4a
to
087ebb2
Compare
jorgefilipecosta
force-pushed
the
update/panel-color-gradient-settings-to-use-dropdowns
branch
from
087ebb2
to
9cb9d65
Compare
jorgefilipecosta
deleted the
update/panel-color-gradient-settings-to-use-dropdowns
branch
|
I had to skip two tests end to end tests because I'm having some trouble fixing them. The functionality works well it is the tests that need an update to account for the UI changes. I'm working on fixing the tests and should have a PR ready soon. |
|
Beta 3 is being released as I write this message, and Beta 4 is not yet confirmed, so we should be mindful of the timeline and concerns about adding improvements so close to RC. As much as I’d personally like to have this improvement in 5.9, could we compromise by removing the |
mtias
added
Backport to WP Minor Release
|
I marked this one as backport to a minor release so we can follow up quickly. If during the remaining testing period of 5.9 this surfaces as an important pain point for people we can reconsider it since it's ready. |
|
Cherry-picked for 5.9.1. |
Mamaduka
removed
the
Backport to WP Minor Release
Co-authored-by: jasmussen <[email protected]>
|
I hope this is the right place for this: As a heavy user of the block editor, editing a lot of blocks per hour, I have one big issue with this change to the color tool: It added two extra clicks to clear the color styles from blocks. One for text, one for background. This adds a lot of unnecessary interactions (and tiresome) when editing many blocks daily. Solution? |
Alternative to: #37053
This PR updates PanelColorGradientSettings to be dropdown-based. The advantage when compared to #37053 is that all the blocks even the ones that use a custom color implementation (navigation, cover, social links, and even third-party) automatically get the new UI.
How has this been tested?
I verified the new color UI appears and works as expected on the paragraph, group, cover, navigation, and social links blocks.
I verified the global styles colors still work as before.
Screenshots