Add Letter spacing and Letter case to Global Styles #43356
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
noisysocks
added
[Type] Enhancement
noisysocks
changed the title
|
Size Change: +680 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for picking this one up @noisysocks 👍
Nice work, everything tests as advertised.
✅ Letter Spacing / Case controls only appear in Heading section under Global Styles > Typography
✅ Letter Spacing / Case controls only appeared for blocks with corresponding support
Come up with solution for unsetting Letter case.
I was about to note this as a problem until I saw your todo item. I've voiced my concerns over the use of a ToggleGroupControl before. I'd like to see a solution to this problem before the switch is made.
We might get away with using the ToggleGroupControl in the block editor given it's within a ToolsPanel that provides us with a means of resetting the control to an indeterminate state. In the site editor's global styles, we only have the "Reset to defaults" nuclear option.
Yeah agree. A design solution is needed. There's some discussion on this in #36735. |
noisysocks
force-pushed
the
add/letter-spacing-and-letter-case-to-global-styles
branch
from
c133c2f to
b06d515
Compare
|
I rebased this to include #43328 and added a None option to the Letter Case control when it's shown in Global Styles as per #36735 (comment). The block inspector continues to use the Tools Panel flow. Kapture.2022-09-09.at.16.56.18.mp4What do you think @aaronrobertshaw @jasmussen? Something I'm unsure about is whether None is the right copy. Perhaps Default would be better as then it implies that the value is inherited from the next level up in the Global Styles hierarchy? |
|
Thanks for the work! Global styles before, had no text letter case option: Now it does, cool, and with a "none" setting:
Letter case in the post editor before:
... and after:
A few things:
I think the minus works good enough to try this out, and "None" is an okay label since it technically matches CSS. It also works to answer the question: "Text decoration?" "None!" 😅 Is this useful? |
I saw that there was a border in the Figma but yep happy to go back to borderless 👍
How would you switch from one of the select options to unconfigured? We don't have a Tools Panel in Global Styles, and the control is a radio meaning that you can't click a selected option to deselect it. What I'm hearing is that #43328 was the wrong approach and that we need to go back to the drawing board and figure out a way to implement click-to-deselect in an accessible way. That way we can distinguish between unconfigured and explicitly unset in Global Styles. I'll look into it. |
|
OK. I opened #44067 as an alternative to this PR. I believe I implemented the UX that you're after @jasmussen where there is a distinction between unconfigured, and explicitly unset. I chose to implement this by partially reverting #43328 and did my best to explain why I think this is okay to do in the description cc. @mirka. |
|
Closing in favour of #44067. |
noisysocks
deleted the
add/letter-spacing-and-letter-case-to-global-styles
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Two changes:
LetterSpacingControl) appear in Global Styles → Typography → Headings.TextTransformControl) to Global Styles, and makes it appear in Global Styles → Typography → Headings.This branch includes the commits in #43328. Once that PR lands I'll rebase this one. In the meantime, only focus on dcfb5ee and c133c2f.
Why?
So that the Typography panel in Global Styles looks closer to what was designed in Figma. See #34345 (comment).
In particular, I'm implementing this suggestion from @mtias #34345 (comment):
How?
Letter spacing was already set up in Global Styles, just not enabled for headings at the global level. To fix this I added a special condition to
useHasLetterSpacingControl.Letter case was not in Global Styles at all. The control already exists so it's just a matter of rendering them in
TypographyPanelin a manner similar to what we do for Letter spacing.Testing Instructions
Screenshots or screencast
Kapture.2022-08-18.at.15.47.50.mp4
To do