-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Block Variations Transforms: Display as icon buttons if each block variation has a unique icon #40036
Block Variations Transforms: Display as icon buttons if each block variation has a unique icon #40036
Conversation
Size Change: +1.03 kB (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
Thanks for testing @jasmussen!
In this case, I think that's expected — the paragraphs selected there have content that's large enough that with the default There's probably a few different ways we could potentially handle that in the future, but I think it might touch on min / max width ideas, so I can't quite think of a good existing issue for it 🤔
Yes, I can also replicate this on trunk, assuming it's what you're seeing (the controls work, but the spacing is broken): From a quick skim I couldn't find an existing issue for it, I've opened one here: #40040 — I'm happy to look into that later this week if no-one beats me to it (I'm wrapping up for the day now) Since you're happy with this PR from a design perspective, I'll add this to the WP 6.0 board to see if we can get it in in time. Thanks for testing! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick green check from design, but would still encourage a confidence boost in the code! Thanks again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @andrewserong!
packages/block-editor/src/components/block-variation-transforms/index.js
Outdated
Show resolved
Hide resolved
@andrewserong @jasmussen yes, the default looks incorrect for most uses cases here. See #39651 (comment) The expectation is things are rendered in a row, otherwise there's no visible change between stack and row, which is super confusing. |
I like the heuristics of the icons, it could work very elegantly! Curious if it breaks apart for social icons and embeds. |
It's working and looking as described in the editor and frontend. ✅ Insert a Group block, and check that each of the transform buttons works correctly Apr-06-2022.08-51-25.mp4With the icon commented out: Here's the difference I'm seeing between stack and row and default. How they work is better demonstrated with background colors. I guess one could argue that it's strange that the transform switches between stack and row when we toggle orientation - seems to make one or the other redundant at first glance. |
packages/block-editor/src/components/block-variation-transforms/index.js
Show resolved
Hide resolved
We currently don't expose the transform to variations for social links (or for embeds), but if we do switch it on, then with this PR it looks like the following: I was originally wondering if we'd need to add a cap to the logic in this PR (e.g. if it'd feel too overwhelming having that many icons), but I actually kinda like it! Happy to explore switching it on (or adding a cap) in a separate PR if folks think it's worthwhile (embeds currently don't all have unique icons so it mightn't be immediately possible there — it renders as a dropdown list by default — so we might need to do some icon wrangling 🤔) |
Thanks for the reviews, folks! 🙇 I'll merge this in once tests pass.
I don't mind the duplication, personally, in that it highlights the explicit relationship between orientation and the block variation — so, whichever control a user updates (variation or orientation) it signals a bit more the expected behaviour. If it's too much, we could always look at hiding the orientation control in a follow-up if need be. |
Sounds good. Definitely not a blocker. Thanks for the explanation too! |
Tiny follow-up bug fix for the Group block variation's |
I've also opened up a fix for the spacing of the color items in the multi-select inspector in #40071 |
That's indeed a curious one! It does mak it way easier to select the variation you want, which to an extent is quite useful. But it's also a lot of logos 😅 |
It sure is! Definitely not an urgent one to figure out (I was mostly just curious to see how it'd look), but I suppose if we did want to use icon buttons to enable switching between variations there, we could introduce a show / hide mechanism (e.g. if greater than a certain threshold then have the buttons collapsed somehow). |
What?
Following on from a comment #39710 (comment) by @jasmussen, this PR looks at swapping out the Transform to Variation dropdown list in the block inspector for a list of buttons (similar to orientation and justification controls).
Because it's possible that there are blocks out in the wild that utilise variation transformations where each variation does not have a unique icon, I've preserved the dropdown list as a fallback in the case that each variation does not have a unique icon.
Why?
The Transform to Variation dropdown doesn't immediately signal to a user what they can do with each variation, hopefully switching to icons makes it a little clearer.
How?
__experimentalBlockVariationTransforms
component and switch to using thegetActiveBlockVariation
selector, as the current approach didn't appear to play well with the Group block's variations which involve anisActive
check that is more tolerant of empty / default values than the existing checks (this fixes a bug where when a Group block was initially inserted, the Group button wasn't highlighted)Testing Instructions
variations.js
file (this line), try commenting out the group icon. Reload the editor and check that the Transform to Variation dropdown menu works as before.Screenshots or screencast
Screengrab of usage (note that switching orientation immediately updates the selected variation):
Screenshots: