Typography Panel: Fix font appearance control width #42795
Merged
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
aaronrobertshaw
added
[Feature] UI Components
|
Size Change: +43 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
|
Tested with German (Deutsch) Before German/English After German/English |
|
Not sure about what's the best compromise — truncating the "letter spacing" label or the contents of the custom select control |
mirka
approved these changes
Aug 1, 2022
There was a problem hiding this comment.
I am in agreement with this change for a couple reasons:
- The initial design mockups did intend for the grid to be two columns with 50/50 width. The fact that it's currently not is just an unintentional "bug" due to the default min-width.
- I think we will soon be forced to put the Appearance control on its own, full-width on a single row. Especially when we upsize these to the 40px variants, the side padding will double to 16px and there will be very little space for the string lengths we'd expect for Appearance values.
aaronrobertshaw
force-pushed
the
update/font-appearance-control-width
branch
from
feb1980 to
a620f6c
Compare
aaronrobertshaw
force-pushed
the
update/font-appearance-control-width
branch
from
a620f6c to
9aa816f
Compare
|
I've spent some time today exploring this a little further. There are languages where the font appearance values already cause the selection text to wrap within the CustomSelectControl's button. In fact, there is nothing stopping the text wrapping to a third row and vertically overflowing the button, which has a fixed height. I couldn't find a real-world example of this though it can be seen if you hardcode a long string into the button via dev tools. While testing further with non-English languages, I found the issue to be larger in scope than initially considered here. For example, take Spanish or French. The letter-spacing field grows beyond the same max 50% column width intended by the design and still gets its label truncated. Demo videoScreen.Recording.2022-08-01.at.2.20.07.pm.mp4The best results I achieved today came through enforcing a 50% width on the panel grid's columns and removing the min-width on the font-appearance control. This provided;
I've pushed a few extra changes, so this can be tested more easily by others, but I'm happy to revert it if there are better solutions or it goes too far. Let me know what you think. Remove font-appearance control's min-width only
Enforcing 50% max-width on tools panel columns as well
|
ciampo
reviewed
Aug 1, 2022
There was a problem hiding this comment.
The points made by @mirka and @aaronrobertshaw sound good to me 👍
| @@ -5,6 +5,7 @@ exports[`ToolsPanel first and last panel items should apply first/last classes t | |||
| display: grid; | |||
| gap: calc( 4px * 3 ); | |||
| grid-template-columns: repeat( 2, 1fr ); | |||
| grid-template-columns: repeat( 2, minmax(0, 1fr) ); | |||
There was a problem hiding this comment.
It would be interesting to see why there's two grid-template-columns rules being applied here, as it may be the symptom of an edge case that we haven't considered yet
There was a problem hiding this comment.
Thanks for catching this one 👍
The first grid-template-columns rule comes from the Grid component. It generates this rule if it has a columns value. This Grid prop gets defaulted to 2 and is typed only to accept a number currently.
The grid template columns defined by the grid component aren't sufficient for the ToolsPanel use case so we add the one with minmax() in this PR.
We originally switched the ToolsPanel to render via a Grid to get some of this layout for free. It seems we'll be benefiting from this less now.
Do you think it would be worthwhile to provide a prop on the Grid component to optionally enforce equal width columns?
There was a problem hiding this comment.
Thank you for the detailed context, as always!
Your explanation and suggestions make sense, but I'd like to discuss these changes with @mirka in a broader sense, since some of Grid's prop are mirroring the exact CSS property (e.g. justify, align, columnGap, rowGap, templateColumns, templateRows...) while other are not (e.g gap, isInline, alignment...). I'd like to first make some clarity on the direction we want to take with Grid (and with Flex too), before adding a new prop — should we expose 1:1 css props, or should we offer a higher level set of props that won't necessarily cover all use cases, or both?
Feel free to merge this PR, if the rest of the feedback is addressed!
aaronrobertshaw
force-pushed
the
update/font-appearance-control-width
branch
from
9aa816f to
001e093
Compare
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.
Related:
What?
Prevents the
CustomSelectControlwithin the font appearance control from forcing a minimum width of130px.Note: The downside here is slightly less width for the items in the select's dropdown. We can enlarge that separately however, for the time being, I've left it to be consistent.
Why?
When the
CustomSelectControlhas a min-width of130px, this forces the typography panel's columns to no longer be a 50/50 split, and once all control labels are capitalized as part of #42789 other typography controls may have their labels truncated.How?
__nextUnconstrainedWidthprop to the font appearance control'sCustomSelectControlto avoid the style forcingmin-width: 130px.Testing Instructions
Screenshots or screencast