Try tabs instead of segmented control for switching between solid/gradient in color panels #41937
Conversation
jameskoster
added
[Type] Enhancement
|
Size Change: -246 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
packages/block-editor/src/components/colors-gradients/control.js
Outdated
Show resolved
Hide resolved
|
|
||
| .block-editor-color-gradient-control__tabs { | ||
| margin-top: - $grid-unit-10; | ||
| margin-left: - $grid-unit-10; | ||
| margin-right: - $grid-unit-10; | ||
| } |
There was a problem hiding this comment.
I wonder if there's a better way to make sure that the Tabs fill the whole width of the sidebar — what about removing the padding from .edit-site-screen-background-color__control and adding it back only to the tab panel's contents?
There was a problem hiding this comment.
I suppose avoiding negative values has some merit, but both implementations would still be exposed to the same risk, no? Just trying to fully understand the implications :)
There was a problem hiding this comment.
In general, there isn't anything wrong with negative margins. In this case, it's more about introducing custom style overrides of the TabPanel component (that implicitly relies on the fact that the popover's content has a padding of $grid-unit-10 px).
Specifically to this case, please ignore my initial suggestion re. removing the padding from — I mixed up this instance (which is rendered in a popover) with the one below (rendered in the sidebar).
It looks like the popover's contents have an 8px margin added directly by the Dropdown component (see the code here) (which I'm definitely not a fan of!)
In this specific situation, I'm not sure if there's a clean way to avoid the style override 🤔
@mirka , any ideas?
There was a problem hiding this comment.
One solution that comes to mind is how the Card component does it. It has a designated <CardMedia> component that you put inside <Card> when you want to break out of the padding. We could provide a similar subcomponent for Dropdown. ("Media" is not a very discoverable name for this, so ideally we'll give it a more literal name like <DropdownContentFullWidth> or something.)
I'm fine with the hacky style override as a temporary thing for this PR to land, and we can make a DropdownContentFullWidth in a separate PR.
There was a problem hiding this comment.
Looking at the mockups in #38277 (cf. some of the implementations in Global Styles ▸ Colors), I'm wondering whether we've thought through the decision to make the tablist break out of the parent padding or not. There seems to be a lot of hierarchical contexts where it would be better not to be full-width. I wanted to check on this point because it does add a good amount of complexity and fragmentation risk if we don't have a clear style guide.
It's totally fine if some top-level tablists are full-width (e.g. Blocks/Patterns, Template/Block, Post/Block). But for things that aren't really top-level, or for components that are reused in multiple contexts such as this ColorGradientControl, things will be a lot simpler if we don't go full-width. Again, your call, but just a consideration from the maintenance/consistency standpoint 🙂 (cc @javierarce)
| .block-editor-color-gradient-control__tabs { | ||
| border-bottom: 1px solid $gray-300; | ||
| margin-bottom: $grid-unit-10; | ||
| } |
There was a problem hiding this comment.
Overriding styles this way introduces fragmentation in the way a component looks across the app, and makes this code less resilient to future design changes.
It would be great if we could use the component as it comes out of the box (or alternatively, we could look at updating the component to fit the new design requirements).
There was a problem hiding this comment.
As far as I know all implementations of TabPanel in core append a bottom border, so we should explore adding it to the component, and removing these styles holistically.
There was a problem hiding this comment.
That sounds like a good plan, at least in principle!
There was a problem hiding this comment.
From a purely design perspective I find the tabs to look a little awkward with no stroke beneath them. But thinking about backwards compatibility I suppose we could add a prop to the component that creates a 'borderless' appearance?
There was a problem hiding this comment.
On one hand, we could add an isBorderless prop (with a default value of true, so that it doesn't introduce a visual change to the component) and replace all "border style overrides" by setting isBorderless to false
But I wonder if this is the right approach:
- I'm not a massive fan of props that are very visually descriptive, as they expose an implementation detail of the design (in this case, showing a border or not) which may not make sense in future iterations
- this pattern of adding props and introduce variants to our components is often a way to mask consistency issues
I'd prefer a scenario in which:
- we investigate how the component is used across Gutenberg (and potentially plugins)
- we assess if the component's design needs updating. In this case, relatively to the bottom border, we should assess if there can be only one version of the component (with or without border) — which would be our preferred option — or if it's absolutely necessary to expose both variants.
- we update the component accordingly and remove style overrides across Gutenberg.
javierarce
added
the
Needs Figma Update
| label={ __( 'Gradient' ) } | ||
| /> | ||
| </ToggleGroupControl> | ||
| { ( tab ) => <p className="screen-reader-text">Selected tab: { tab.title }</p> } |
There was a problem hiding this comment.
I believe what needs to happen here is to provide a function that receives the currently selected tab as an argument and render the tab panel for it. Basically the TabPanel is going to manage our currentTab state from now on. That is how it handles the a11y concerns for us.
Since it looks like we're going to use the tab panel content outside of a TabPanel context as well, perhaps we could do some light refactoring along these lines:
const tabPanels = {
[TAB_COLOR.value]: <ColorPalette foo={bar} />;
[TAB_GRADIENT.value]: <GradientPicker foo={bar} />;
}; { canChooseAColor && canChooseAGradient && (
/* Note that we don't need the `onSelect` anymore */
<TabPanel
className="block-editor-color-gradient-control__tabs"
tabs={ TABS_SETTINGS }
>
{ ( tab ) => tabPanels[ tab ]; }
</TabPanel>
) }
{ ! canChooseAGradient && tabPanels[ TAB_COLOR.value ] }
{ ! canChooseAColor && tabPanels[ TAB_GRADIENT.value ] }There was a problem hiding this comment.
Oh yeah, that makes sense :)
I tried to implement this as you suggested but my designer brain has obviously messed something up because the panels aren't rendering for me 🤔 I don't suppose you could take a look?
There was a problem hiding this comment.
Ah yes, of course! I wrote that code snippet without running it so something is likely wrong 😂 I'll rework the code and push it directly to this PR branch then 👍
There was a problem hiding this comment.
Ok, done ✅ Passing the torch back to you!
packages/edit-site/src/components/global-styles/screen-color-palette.js
Outdated
Show resolved
Hide resolved
| @@ -75,7 +75,7 @@ function ScreenLinkColor( { name } ) { | |||
| ) } | |||
| /> | |||
|
|
|||
| <TabPanel className="my-tab-panel" tabs={ tabs }> | |||
| .block-editor-color-gradient-control__tab-panel { | ||
| border-top: 1px solid $gray-300; | ||
| } |
There was a problem hiding this comment.
FWIW, after working on this PR, I'm now a little apprehensive about adding in this ad hoc border. The Global Styles ▸ Colors menu already contains several tabbed color panels that have different implementations, easily leading to style fragmentation. See also #41976 which seems to have landed recently. Your call, but it might be better to leave the border off for now and address the border issue in a separate PR.
|
|
||
| .block-editor-color-gradient-control__tabs { | ||
| margin-top: - $grid-unit-10; | ||
| margin-left: - $grid-unit-10; | ||
| margin-right: - $grid-unit-10; | ||
| } |
There was a problem hiding this comment.
Looking at the mockups in #38277 (cf. some of the implementations in Global Styles ▸ Colors), I'm wondering whether we've thought through the decision to make the tablist break out of the parent padding or not. There seems to be a lot of hierarchical contexts where it would be better not to be full-width. I wanted to check on this point because it does add a good amount of complexity and fragmentation risk if we don't have a clear style guide.
It's totally fine if some top-level tablists are full-width (e.g. Blocks/Patterns, Template/Block, Post/Block). But for things that aren't really top-level, or for components that are reused in multiple contexts such as this ColorGradientControl, things will be a lot simpler if we don't go full-width. Again, your call, but just a consideration from the maintenance/consistency standpoint 🙂 (cc @javierarce)
|
I've removed the ad hoc border, and the negative margins in the popover. I agree it would be best to address this at the component level first. |
|
Thanks for the review :) I'll merge once another designer has signed off.
I'll open an issue for us to discuss this in more detail. |
|
This looks good to me, nice job! |
|
Apologies for coming back here after almost a month 😅 — but I much preferred the segmented control, and would want us to have that also for inline text colors:
I don't dislike the tabs especially in global styles, though I still think the segmented control worked better there. The tab by virtue of going edge to edge acts as a separator or a new sub-section, when in fact it is contextual to the controls above and below both. While I'd personally prefer us going back to the segmented control, I also don't mind trying this out for a bit. But we should at least change "Solid color" to just read "Solid". I'll spin up a PR. |
|
I created #42918 as a followup to renaming, but learned that there are some awkward padding issues. I do see those raised in this thread, I wonder if that regressed? Would going back to the segmented control simplify some of this? |
Especially if we want the full-width color button to also happen, I'd say it would take more work to achieve everything correctly with the ToggleGroupControl. The main hurdle with using the ToggleGroupControl as a I'm not entirely convinced that a radio control and a tab panel should look the same (e.g. what happens if you need to put a radio control inside the tab panel?), but if that's the direction we're going, it is technically possible. |
Just to be sure, that means when we had the ToggleGroupControl for solid/gradient instead of the new tab-group, the HTML was incorrect? |
Exactly. HTML-wise, that was just a radio control preceding the rest of the color controls. No hierarchy or indication that it was a tab panel switcher. |
Thank you for explaining. In various forms isn't it reasonable enough to have a radio group contextually affect subsequent options? I don't think of the segmented control (ToggleGroupControl) as being conceptually related to a tab panel switcher, so unless there's some semantic reason not to use it, I'm unsure it's a problem. But I'm happy to learn here if I'm wrong. |
|
Radios don't make sense in some flows, the inline text color options your shared in this thread are a good example:
You don't select text or background, you can select both. |
|
In that case tabs are more appropriate, indeed, but I feel the original point for customizing a single color still stands. It's not a very very strong opinion — things look okay. It's just not clear to me that the tabs are better for the color panel. |
|
There may be an argument to support both, dependent on situation. In the example above, and when editing palettes in global styles, I think tabs make more sense. When applying a background color (solid vs gradient) to a specific block, radios / segmented control may be preferable. |
When picking a color, where both solid colors and gradients are available, their visibility is toggled through a component that resembles a segmented control:
One drawback I find with this approach is that it is not a consistent implementation of the segmented control pattern when compared with other instances in our UI. When choosing a font size (for example) a selection equates with an actual value, whereas with colors the value isn't set until a color swatch is clicked.
To promote consistency we might instead use tabs for switching between the solid / gradient palettes:
tabs.mp4
This feels like an appropriate use case for the TabPanel component:
A somewhat related consideration is the tab labels. Since they toggle the visibility of different palettes, and palettes typically contain more than one color, I wonder if the labels should be plural?
This is probably something we could adjust here as well, if it is deemed appropriate.