Duotone: add block controls on the inspector #49838
Conversation
| /> | ||
| </InspectorControls> | ||
| <BlockControls group="block" __experimentalShareWithChildBlocks> | ||
| <DuotoneControl |
There was a problem hiding this comment.
@youknowriad we tried to use the StylesFiltersPanel here too but it doesn't work outside of the inspector context, so we decided to leave it as it is right now.
There was a problem hiding this comment.
yes, this is a new use case (block toolbar). Maybe there's a way to make these controllers agnostic here too by providing a specific Wrapper prop, did we try that?
There was a problem hiding this comment.
yes, we did that, and the ToolsPanelItem would not render, isShown is false in that context
There was a problem hiding this comment.
I guess it's because of a missing ToolsPanel container within the "wrapper", that component seems mandatory. We can probably include it in the block toolbar too though
MaggieCabrera
added
the
[Feature] Design Tools
|
Size Change: +2.15 kB (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 6e7e6ec. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4744963811
|
| @@ -32,6 +32,7 @@ const StylesTab = ( { blockName, clientId, hasBlockStyles } ) => { | |||
| label={ __( 'Color' ) } | |||
| className="color-block-support-panel__inner-wrapper" | |||
| /> | |||
| <InspectorControls.Slot group="filter" /> | |||
There was a problem hiding this comment.
@jasmussen is this a proper order for the filters? I put it after color because it's what made sense to me
|
This looks good, thank you, here's a screenshot of duotone in the inspector:
To note, one of the reasons it's called "Filters" is that I expect additional filters to land in the future. That said, until we actually do have more filters, it might be best to call it still "Duotone", we can always rename. One thing I'm missing from the mockups, though, is the ItemGroup+Flyout combo, this one:
This is mainly to compress the space usage in the inspector. There are a lot of other valuable options there, including border and radius, and even if we could push "Duotone" down below, it wouldn't really help the overall glancability. Would it be possible to move this into such an ItemGroup+Flyout combo? I know that the Box shadow component in Global Styles recently did the same, perhaps there's some code there that can be repurposed? |
Yeah, I think we can do that |
Should we have the ItemGroup+Flyout combo in Global Styles too @jasmussen ? As in, should I edit |
Regardless of what we do, I think we should be consistent and just modify the same component for both IMO. |
I think in this case we can mimic what the colors panel is doing. It will look a little bit lonely with just one kind of filter, I suppose |
I think that's fine for now, and can be a reason to call the panel "Duotone" for now. And yes, we can have it be the same in global styles an the inspector. |
|
I think this can get the code reviewed already. I'm thinking we shouldn't change how the toolbar controls work on this PR, but in a follow up instead. Post editor:
Global Styles:
@youknowriad what would be the best way to disable the custom duotone controls only for GS? that requires a more involved fix that is not relevant to this one. If you don't think we should do that, I'm happy to just remove the option from this panel until that is addressed. |
|
Personally, I'm often for "consistency". So if custom duotone is something that is supported already by theme.json (and user theme.json), I don't see why we should disable it for global styles UI. |
The thing is, it is not! right now it only works on a block level, on theme.json you can only assign presets to the specific blocks. That's a known bug, and needs addressing separately. |
that sounds good to me! I'll get to fix the custom duotone on gs/theme.json afterwards so this will be a temporary thing |
|
Taking this for a spin, it feels great! Thanks so much for putting it in the itemgroup, this feels validating to me in practice. Here's Global Styles > All blocks > Images > Filters > Duotone:
Note that when the flyout is invoked from the inspector, it should use the default popover style. Light gray border, small drop shadow underneath:
The almost black border is meant to only be applied to popovers that open from the block toolbar:
Last I worked on this, the prop was called Post editor generally looks good, the compression in the inspector yet again validates the use of the itemgroup:
Noting here that duotone can be applied to at least the image block, even in the setup state. This is intentional, and should make the gray placeholder look like this:
However I noticed this filter is also applied in the selected state for this block, both i this PR and in trunk:
This used to work, so it's a regression. It's not your burden to fix, as it's not the fault of this PR, just noting it as a result of testing. Nice work! |
| @@ -27,7 +27,13 @@ export default function FiltersPanel( { name } ) { | |||
| inheritedValue={ inheritedStyle } | |||
| value={ style } | |||
| onChange={ setStyle } | |||
| settings={ settings } | |||
| settings={ { | |||
There was a problem hiding this comment.
I wonder if we should memoize this object (depends on how it's used internally)
Yes, it was a variant, I made it look like the rest. Thanks for reporting the bug on the image placeholders! |
| placement: 'left-start', | ||
| offset: 36, | ||
| shift: true, | ||
| className: 'block-editor-duotone-control__popover', |
There was a problem hiding this comment.
It's not great when we use "classnames" as reusable components. In the ideal scenario, the styles should be defined in the same place where the classname is used.
There was a problem hiding this comment.
yes! this I think can be refactored when we work on using the filter panel for the toolbar controls, I will make a note to do just that
| { indicators.map( ( indicator, index ) => ( | ||
| <Flex key={ index } expanded={ false }> | ||
| { indicator === 'unset' || ! indicator ? ( | ||
| <ColorIndicator className="block-editor-duotone-control__unset-indicator" /> |
There was a problem hiding this comment.
I saw this check and this class in a couple places, It seems to me that the DuotonePicker already understands the unset value, so it's not far fetched to say that the DuotoneSwatch should also understand the unset value and do the right thing internally instead of duplicating the styles and the classname every time.
There was a problem hiding this comment.
I will tackle this one when refactoring the DuotoneControl component too, good catch
packages/block-editor/src/components/global-styles/filters-panel.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/global-styles/filters-panel.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/global-styles/filters-panel.js
Outdated
Show resolved
Hide resolved
|
When I look at a block that already has a Duotone set, the setting doesn't appear to be selected: Similarly for Global Styles, the default color from theme.json isn't populated: In trunk it looks like this: |
|
@scruffian for the global styles unset value is a known issue that was solved here #49806 Edit: you may be referring to something else though :) |
@jasmussen I'm catching up with pings after some AFK — I can't actually recall the details of this conversation. Would you be able to create a new issue to track these requirements? Thank you! |
|
@ciampo thanks for catching up. Before I open a new issue, I found a little more context in #45137, where the "isAlternate" is refactored/deprecated in favor of a variant. But it boils down to this, you can have two styles of popovers. This one: Notice how it opens from the block toolbar and has the same black borders as the block toolbar. This one:
Notice how it doesn't open from the block toolbar, and has the light gray borders, plus a drop shadow. The thing is, a developer should not have to choose a variant if we can automate this for them. The gray with box shadow can be default, but if something opens from the block toolbar, it should always be the dark version. That way we avoid this issue where someone forgot the variant:
Worth a ticket, I feel like there may have already been one? |
|
Thank you for the great explanation around the context for this, @jasmussen ! I think this could be something that we could do via context APIs
You're right, @mirka had already opened one: #45246 I'll go ahead and update it with the additional context from your comment above |
What?
This PR aims to show the new
StylesFiltersPanelin the inspector.A follow up to this PR will simplify the contents of the toolbar controls and separate the duotone filters in the inspector by origin like the mockups in #39452.
To do:
Part of #39452
Why?
Right now the toolbar is the only place to edit the duotone filters on a block level. This PR brings the UI in line with other controls and paves the way for future simplification of the toolbar options.
How?
By leveraging the new panel
StylesFiltersPaneland displaying it using a dropdown. This alters how the panel looks on Global Styles right now tooTesting Instructions
I'm using Skatepark because it applies duotone on all images, but testing on block theme that doesn't is also appropriate.
Check the new panel in the inspector while editing an image and that you can apply the filters in the same way as you would from the toolbar.
Test the same but on global styles, applied to all images on the site.
Screenshots or screencast