PaletteEdit: Use ItemGroup and Item, and avoid custom styles
#66164
Conversation
tyxla
added
[Type] Enhancement
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -266 B (-0.02%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
packages/components/CHANGELOG.md
Outdated
| @@ -19,6 +19,8 @@ | |||
|
|
|||
| - `Modal`: Modal dialog small improvement for elementShouldBeHidden ([#65941](https://github.com/WordPress/gutenberg/pull/65941)). | |||
| - `Tabs`: revamped vertical orientation styles ([#65387](https://github.com/WordPress/gutenberg/pull/65387)). | |||
| - `ItemGroup`: default `isBordered` and `isSeparated` to `true` ([#66164](https://github.com/WordPress/gutenberg/pull/66164)). | |||
There was a problem hiding this comment.
I'm not sure I understand why is this change needed — couldn't we just set the two props to true in PaletteEdit?
(also, technically this is a breaking change and we'd normally try to avoid that as much as possible)
There was a problem hiding this comment.
As described in the PR, I'm fine with a breaking change for a few reasons:
- The sole usage of this component is in
PaletteEdit - The default non-bordered behavior of the component is awkward
- The component is experimental, so it's perfectly OK to set better defaults, even through a breaking change.
Related to the CHANGELOG entry, if this is a breaking change to an experimental API, do we treat it as a breaking change? It wasn't an official public API, after all. Happy to move it to "Breaking Changes", but to me it makes sense if that's reserved to changes to components that aren't private, experimental or unstable.
There was a problem hiding this comment.
The sole usage of this component is in
PaletteEdit
That's definitely a point to keep in mind when judging the impact of these changes — although I haven't looked on third-party usage.
The default non-bordered behavior of the component is awkward
That's absolutely true. In general, there's a lot of improvement work on ItemGroup on the horizon
The component is experimental, so it's perfectly OK to set better defaults, even through a breaking change.
While the component is marked as experimental, it is de facto considered stable by some folks given that it was included in a WordPress release — hence why I questioned this change in the first place.
Having said that, we can definitely introduce this change if we think it improves the component — we should provide a dev note to let folks know (with an example of how to revert to previous styling, though).
Related to the CHANGELOG entry, if this is a breaking change to an experimental API, do we treat it as a breaking change? It wasn't an official public API, after all. Happy to move it to "Breaking Changes", but to me it makes sense if that's reserved to changes to components that aren't private, experimental or unstable.
I'm tempted to mark it as a Breaking Change, since I believe that recently we've been treating the "Experimental" section of the changelog as referred to the truly experimental (ie. exposed via private APIs) components. But I don't have a strong opinion — @mirka , do you have any thoughts?
There was a problem hiding this comment.
That's definitely a point to keep in mind when judging the impact of these changes — although I haven't looked on third-party usage.
We should be good: https://wpdirectory.net/search/01JAAG2CQV99VX0PZX2PER2BFA
That's absolutely true. In general, there's a lot of improvement work on ItemGroup on the horizon
Yep, thanks for confirming, I actually did link that in the PR description already.
While the component is marked as experimental, it is de facto considered stable by some folks given that it was included in a WordPress release — hence why I questioned this change in the first place.
Given that the name hints it is experimental, and docs are clear, I think the limitations here are self-imposed, and we need to be more flexible. Given that I don't see any other usage of that component, I'm confident to make this change at this time.
Having said that, we can definitely introduce this change if we think it improves the component — we should provide a dev note to let folks know (with an example of how to revert to previous styling, though).
Good point, happy to provide a dev note if we end up landing it.
I'm tempted to mark it as a Breaking Change, since I believe that recently we've been treating the "Experimental" section of the changelog as referred to the truly experimental (ie. exposed via private APIs) components. But I don't have a strong opinion — @mirka , do you have any thoughts?
Happy to be flexible with whatever the consensus is - let me know.
ciampo
added
the
Needs Dev Note
|
In general I've been treating the
I'm not sure I understand this correctly, because I'm seeing a good number of I do lean toward keeping the default-flipping a separate discussion, especially since we are already planning to overhaul the component anyway. We can probably also do more to improve the situation through documentation — for example making the bordered & separated example more prominent. Or even making the default story have the two props enabled. We usually showcase the default state, but in this case I can see the advantages. The other changes in this PR sound good though 👍 |
tyxla
force-pushed
the
update/item-group-defaults-and-palette-edit
branch
from
677c676
to
72b632a
Compare
tyxla
changed the title
ItemGroup with improved defaults, Item, and avoid custom stylesItemGroup and Item, and avoid custom styles
tyxla
force-pushed
the
update/item-group-defaults-and-palette-edit
branch
from
72b632a
to
bd4302d
Compare
This would be my preferred short-term solution. With the long-term solution being that we rework the component to follow the new specs — and that's where applying breaking changes (or introducing a new version altogether) would make more sense. |
ciampo
removed
the
Needs Dev Note
There was a problem hiding this comment.
Since this uses the
Itembuilt-in spacing, this introduces some visual changes, so I could use a look from @WordPress/gutenberg-design. I'm happy to address any suggestions while here, but they'll need to be done insideItem, and AFAIK, we're aiming to work on this separately (see #64445).
I noticed that originally PaletteEdit was using Item, but was changed in #38035 to a div + custom styles to fix a double-border bug (cc @jorgefilipecosta ) — although these hover + selected styles don't seem to be there anymore (probably as the result of #62753)? (this can be tested especially when setting the canOnlyChangeValues to true).
Not sure if we want to make any tweaks there? But since this seem to be same on trunk, any changes could happen in a follow-up.
Apart from that, changes LGTM 🚀
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
Sounds good; I'm happy to make any changes in a follow-up. |
What?
This PR updates
PaletteEditto make use ofIteminstead of implementing custom styles.Why?
PaletteEditcurrently usesItemGroupin an odd way, and defines its ownItemwith a bunch of custom styles. For better consistency, when usingItemGroupwe should be usingItemcomponents.How?
PaletteEditto useItemGroupwithItem, instead of building its own customPaletteItemPaletteEditpalette item.Since this uses the
Itembuilt-in spacing, this introduces some visual changes, so I could use a look from @WordPress/gutenberg-design. I'm happy to address any suggestions while here, but they'll need to be done insideItem, and AFAIK, we're aiming to work on this separately (see #64445).Testing Instructions
PaletteEdit:Testing Instructions for Keyboard
Same
Screenshots or screencast
Storybook,
PaletteEdit, default story, editing palette:Site Editor, global styles, colors, editing palette: