-
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 Editor]: Add flex-wrap
control to flex
layout
#36003
Conversation
Size Change: +1.67 kB (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
This is great, and I can tell it will solve many of the responsive issues we've had with Twenty Twenty-Two header patterns: WordPress/twentytwentytwo#89 Rather than "Flex Wrap", I wonder if we should just call this "Wrapping"? |
Instead of using "flex" and "wrap" maybe we can explain this better with more straight-forward language. Also, as this is a binary (on/off) setting, maybe the segmented control is the wrong UI. Instead, maybe a switch will do what we need: I'm not entirely sure what the default should be; Do you turn off "multiple lines" or turn on "multiple lines" — a lot of those decisions should be based around actual usage and general needs. |
I like @shaunandrews suggestion. I think that's the only information end-user need to use this feature. The default value of Note to self: Browser defaults to |
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.
Thanks for working on this, Nik.
The change looks great codewise. I will leave the final ✅ to more design-savvy folks 😄
CleanShot.2021-10-28.at.11.53.05.mp4
I wanted to leave a small note on this feature. On the one hand, it solves an immediate problem at hand, and gives power to the Row block and similar flex blocks, power that can solve responsive problems in an intrinsic way (see also #34641). At the same time, the feature itself is rather complicated for anyone unfamiliar with CSS. To an extent, the Row block can be thought of as a horizontal version of the group block: why wouldn't it always wrap? By virtue of being a property of the flex type, this property also gets added to every flex block, including as demoed (thank you George!), Social Links. And therein you could get into trouble: disallow wrapping on it, and increase the gap. What happens? CSS and HTML has always allowed you to get into trouble. We can't entirely prevent that from happening in the block editor. But because of the complexity of the wrapping feature, and because of the trouble it can get you in, before we land this we need to be absolutely sure that this is a feature we want to have, and that we have considered all other alternatives before it. And if we do end up decicing: yes, this is a valuable feature, next we must manage the prominence of it. In context of the above, I would reduce the prominence as much as possible, due to it being in superuser territory. If we could change the Layout panel to a Tools Panel, then hide the wrapping feature in the ellipsis and never show it by default, we'd help balance the complexity, yet still let it be available for those who seek it out. (Balancing the prominence is also an argument for using the toggle 👍 👍) Finally, there's something about the spacing and margin on this one that feels imbalanced: It's as if wrapping is related to justification when it's not. This is a bit of a general problem with toggles that we need to solve in light of #27331. It may just be to increase the margin? |
These are great concerns @jasmussen, thank you! I would love some folks with more experience in the css implications to share their thoughts. Personally, after reading Joen's comments I might lean on landing this after the ToolsPanel change has landed 🤔 |
It seems like the alternative here would be to intelligently handle wrapping by default. I find it a little hard to wrap my head around that though — it would take a lot of trial and error. I'm not sure if this is a helpful answer or not, but should this be the sort of feature we launch without any UI? Since as you note, folks can get into a lot of trouble with it, maybe it's only available for themes/patterns to use, until we come up with a safer/automated solution? One that will get folks into less trouble? |
I don't think its that complicated, nor do I think it will "get people into trouble" — at least not any more than any of the other settings on this block. (I'm staring directly at the bonkers "Inherit default layout" switch, which feels completely broken here...) I think the langue could probably do a lot to help explain the setting. I went so far as to mockup a visual representation of the setting, but I'm not entirely sure its necessary. To me, it's a switch that I'd toggle on and off a few times and see what happens. It's probably worth noting that this section will eventually make use of the Tools Panel, which means this switch will only be visible when needed. |
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.
This will unblock us for Twenty Twenty-Two header issues, so I think we should we should add it for now. We already don't show any flex UI until that setting has been manually added to a block, so we have room to iterate on this and automate it moving forward.
You mean |
Yes, I mean flex in general. All of the flex controls are somewhat hidden until they're needed, so I feel ok about adding this toggle there too. |
Resolves: #35525
This PR just adds a
flex-wrap
control inflex
layout.