Allow setting Letter case and Decoration to 'None' and add Letter case to Global Styles #44067
Conversation
noisysocks
added
[Type] Enhancement
|
Size Change: +255 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
|
Thank you for your patience on this one. The behavior in this PR feels correct: there's an explicit "none" added to the controls, you can toggle and untoggle (so you can go back to unset unlike the radio behavior). We have a visual challenge, though. Each dark toggle button should be exactly 32x32px, but they are 36px tall and flexing in their width:
If I do a little inspector hacking I can get them to the right sizes, which introduces a different problem, that there isn't room for 3 + 4 buttons:
A few solutions to this one:
Another option is to allow the two columns to be flexible. Mockup:
CC: @javierarce @jameskoster in case you have thoughts, and @ciampo @mirka for thoughts from the component side. The near term solution, probably, is to allow the text decoration to fill a full row, and to get the buttons to be the correct 32x32px sizes. Does that sound okay? |
|
Just connecting some dots here, this PR seems to overlap: #43993 CC: @draganescu @scruffian |
|
I'll absolutely close #43993 for this one here which is a complete implementation since being able to un-select is essential to preserve theme styles as they are w/ no user intervention. |
|
Personally I find 24x24 buttons awkward to click, especially on mobile. And the difference in height between buttons / inputs creates unnecessary tension in the overall appearance. Since we're committed to making all inputs 40px tall, I would be inclined to do the same with icon buttons. If that means some controls (like this one) occupy a single row, then I think it's worth it. In the future we could consider making the Inspector wider to better accommodate multi-column layouts. |
Just to clarify here, I don't think that means the 32x32px buttons must become 40x40 — or that the toggle control has to be 40px tall suddenly. The buttons can remain 32x32, but inside a 40px wrapper, so the spacing remains right. |
|
That's the reason I think it's worth considering making the icons 40x40. It gives us this:
Which feels more balanced than:
|
This is a great pull, thanks! It clears up the HTML concerns, which feels like a big step forward. However I feel like this solution doesn't fully address one of the main takeaways of the article (and Léonie's follow-up), which is that we have to be careful about not mismatching user expectations. Having UI components that pretty much look the same (i.e. a row of square icon buttons with no enclosing border) that are sometimes mutually exclusive and sometimes not, sometimes de-selectable and sometimes not, seems like it would result in a lot of mismatched expectations within the app. So from a design standpoint, I would advocate for a bit more clarity around these affordances before we go further. We have three possible combinations, so for example a consistent system could look like:
It's probably not super important to visually afford mutual exclusivity because it's obvious after the first interaction, but having a visual differentiator for de-selectability seems important to me. Thoughts? From the components maintenance standpoint, we will extract whatever design pattern that's decided on into the components package so it can be reused elsewhere in a consistent way. Ideally that componentization happens before this PR merges, but if it's urgent we can clean it up later 🙂 |
Forgot about your PR sorry! Props @draganescu ❤️
A single row sounds good to me. I think right now the buttons are 36px with a 2px margin but I can make them 40px to align fully with the inputs.
I love this and completely agree! 💯 I'd add that only when there is a border (2) should the group be in a single tab stop.
Happy to look at doing this in a components-first way. Were you thinking that there would be separate components for the three patterns above, or could all three patterns be handled by |
|
Lots of good thoughts here. Happy to try the suggested outline. Outside of the components work, let's allow the 4-button group a whole row for now. I don't think we should touch any dimensions at this point, meaning 32x32 remains the toggle size so it matches up with these buttons:
|
It's a tough call but I'm leaning toward handling everything in ToggleGroupControl with two extra props. I imagine it would be confusing for a dev to pick out the correct component to use if we split them 🤔 I'll start working on this during the coming days and see how it goes. Might have to split somehow if the internal logic feels untenable. |
Cool yep. I've changed the button size to be 32px with 4px of top/bottom margin so that it's centred with the 40px inputs. I've also changed Letter case to take up a full row. Here's how it looks now:
Legend, thanks! 🙌 I'm happy to help—ping me. I think in that case then let's proceed with this PR as a smallish win (it's nice to have consistency between block settings and global styles) and then work on trying to consolidate everything around using |
There was a problem hiding this comment.
Nice work, looks good!
The explicit none appears to work as intended. It's still unclear if the minus is the best longer term icon, but that should't block this from landing, as it's an important tool. Thank you for your patience on this one.
I'd love a code review, but for the behavior, let's try this.
There was a problem hiding this comment.
It's great to see the ability to "toggle off" items in these controls return. 👌
✅ Letter case appears in the Heading section, but no other "root" level sections
✅ Letter case appears in the Typography panel for blocks that support these controls
✅ Modifying Letter case works
✅ Controls reflect none selection when set via theme.json
✅ Storybook examples for text transform/decoration controls look & function correctly
❓ Letter case buttons are squashed (not square). Occurs in both block editor and Global Styles panels.
Overall this PR works pretty well for me. There was just a small styling issue that I think we need to tweak first before landing this.
| Block Inspector Controls | Global Style - Block | Global Styles - Heading |
|---|---|---|
 |
 |
 |
When I view the TextTransformControl in both the site or block editor, the buttons are not square. They are in the Storybook examples. I believe this is due to the Letter case control not being set to span both columns and thus has its width constrained.
To change that I believe we'd need to remove the single-column class from the typography support hook's use of the control. Also, the controls in the Global Styles panels appear to use the .edit-site-typography-panel__full-width-control class. I've left an inline comment on that below.
packages/block-editor/src/components/text-transform-control/index.js
Outdated
Show resolved
Hide resolved
|
Just to echo Aaron's comments, this is looking good except for the squashed buttons:
Ideally the toggled state is 32x32 and if letter case can't fit in a single column, it's okay if it has to span two columns. |
|
🤦♂️🤦♂️🤦♂️ I forgot to |
…aria-pressed again
noisysocks
force-pushed
the
try/go-back-to-buttons-for-text-decoration-and-transform
branch
from
2f90a67
to
3e98495
Compare
|
OK try this now. I rebased it to include #44142, implemented @aaronrobertshaw's suggestion about adding a |
noisysocks
added
the
Backport to WP 6.7 Beta/RC
noisysocks
deleted the
try/go-back-to-buttons-for-text-decoration-and-transform
branch
…e to Global Styles (#44067) * Make TextDecorationControl and TextTransformControl use buttons with aria-pressed again * Add option for 'none' text-transform and text-decoration * Add Letter Case to Global Styles * Make buttons 32px * Make TextTransformControl take up full row * Add back className
|
I just cherry-picked this PR to the wp/6.1 branch to get it included in the next release: 853051b |
ockham
removed
the
Backport to WP 6.7 Beta/RC
What?
Alternative to #43356. Partially reverts #36735. Closes #36735 and addresses one of the items in #34345 (comment).
This PR does three things:
TextTransformControlandTextDecorationControlinstead of aToggleGroupControlTextTransformControlandTextDecorationControlwhich allows user to settext-transform: noneandtext-decoration: noneTextTransformControlto Global StylesWhy?
We want to add
TextTransformControlto Global Styles. See #34345 (comment).We also want to allow the user to set
text-transform: noneortext-decoration: noneexplicitly. This allows users to override theme styles at a block level. See #42766 and #43356 (comment).So, using
TextTransformControlas an example, the control should support these options:text-transform: none)text-transform: uppercase)text-transform: lowercase)text-transform: capitalize)Displaying all five options is overwhelming. It is much more intuitive for users if we can display only four buttons (None, Upper case, Lower case and Capitalize) and implement Default using a click-to-unselect mechanism or the ellipsis menu in the Tools Panel. Unfortunately though,
ToggleGroupControldoes not support click-to-unselect as it usesradio, and Global Styles does not use Tools Panel.We switched from a group of buttons to a
ToggleGroupControlin #36735 for two reasons:TextTransformControlandTextDecorationControlwould have updated 40px styling.role=radiothat these options are mutually exclusive.(1) can be implemented by styling the group of buttons.
I've been reading about (2) and I don't believe it's actually a big concern. There is little practical difference in terms of what is announced by a screen reader between a group of buttons with
aria-pressedand a group ofradios. Moreover, we should use the markup and semantic roles that most closely resembles the UI. In this case, I believe thatTextTransformControlandTextDecorationControlis a group of buttons, because:TextDecorationControlcould in theory support multiple values e.g.text-decoration: underline strike-throughhttps://lea.verou.me/2022/07/button-group/ is a good article that I found on this subject.
Testing Instructions
npm run storybook:devand see the stories forTextTransformControl/TextDecorationControl.Screenshots or screencast
Kapture.2022-09-12.at.17.45.56.mp4