-
Notifications
You must be signed in to change notification settings - Fork 201
fix(actionbutton): prevent diacritic clipping in thai #3256
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
fix(actionbutton): prevent diacritic clipping in thai #3256
Conversation
🦋 Changeset detectedLatest commit: 68da4c2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🚀 Deployed on https://pr-3256--spectrum-css.netlify.app |
File metricsSummaryTotal size: 4.30 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailsactionbutton
actiongroup
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
This addresses the core issue of preventing the clipping, although it's very tight for XS per current design (non-blocking issue). We may want to file a suggestion for design to slightly increase the padding-block space (min button height) for XS by a couple pixels so that the diacritic marks have a bit more breathing room. |
Oh yes, great catch. In comparison, what if we went with a line-height fix instead? It looks like maybe it's a little better. A solution with line-height was another option in some recent fixes for picker (but I can't remember why we didn't end up going with it). Maybe that's a better solution here- what do you think? It also looks like maybe that span no longer overflows with line-height. |
@marissahuysentruyt since we do not allow line-wrapping, line-height does alleviate the issue in a slightly more "correct" way since it changes the way the text-box is evaluated by increasing the room for the characters. However, if we allowed line-wrapping, then the larger line-height, especially pixel-based, would not be ideal, so padding may be a preferred solution in other cases. For future reference back on this PR, it's probably also worth noting that the clipping is only happening due to |
08c6aed
to
a03d304
Compare
Can we add a test case to the actionbutton with an example of this issue so we can prevent regressions in future? |
a03d304
to
4b052ab
Compare
4b052ab
to
5eacbd0
Compare
5eacbd0
to
81ebf2f
Compare
components/actiongroup/index.css
Outdated
&.spectrum-ActionGroup--justified .spectrum-ActionGroup-item { | ||
/* flex: unset removes the flex: 1 set on `.spectrum-ActionGroup--justified .spectrum-ActionGroup-item`, | ||
in order to keep the action buttons from shrinking */ | ||
flex: unset; | ||
} |
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.
@5t3ph I know you suggested that maybe we should use flex-grow: 1
as opposed to the flex: 1
(which would also set flex-shrink which was probably causing our squish-ness), however, I was struggling to not undo the horizontal justified behavior.
I tried flex-grow: 1
in a couple of places:
.spectrum-ActionGroup--justified .spectrum-ActionGroup-item {
flex-grow: 1;
}
// OR
.spectrum-ActionGroup--vertical {
gap: var(--mod-actiongroup-vertical-spacing-regular, var(--spectrum-actiongroup-vertical-spacing-regular));
display: inline-flex;
flex-direction: column;
&.spectrum-ActionGroup--justified .spectrum-ActionGroup-item {
flex-grow: 1;
}
}
The first selector fixed the vertical justified button heights (from 36px to the correct 32px), but then unjustified the horizontal justified action group:
The second selector....didn't do anything? The action buttons were still the incorrect 36px, (and the icon-only aren't tall enough).
With the new selector here with flex: unset
, the horizontal justified remains the same, AND the vertical justified now is not squished and the proper 32px.

Am I missing anything with flex-grow
? I know you mentioned min-block-size
might be redundant, but would you rather that than the flex: unset
? I haven't tried anything with that yet.
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.
After our pairing, we realized I just didn't understand what should be happening in a justified actiongroup! flex-grow: 1
is now up: 85656a0
81ebf2f
to
85656a0
Compare
85656a0
to
b06ebe0
Compare
b06ebe0
to
e67ad79
Compare
e67ad79
to
57a2afa
Compare
- add line-height to .spectrum-ActionButton-label - add changeset - add test case for Thai language
- for justified action groups, specify that the desired effect is for them to grow to fill the available space (as opposed to allow allowing flex-shrink) - add changeset
57a2afa
to
68da4c2
Compare
Description
Recently, another bug was captured in relation to the diacritics and tone marks getting cut off in the action button when the text is Thai: https://jira.corp.adobe.com/browse/CCEX-131368.
This PR attempts to fix that bug by adding padding to the action button's label, allowing for the diacritics to overflow into the padding space, and no longer get cut off. A similar solution was implemented recently for picker (#2914)
If we decide that adding padding is causing VRT diffs or other unexpected issues we want to avoid, we could explore using line height as well (a PR for the same picker issue mentioned above was closed in favor of the padding fix instead: #2913)
The 2 commits related to adding and then removing the Thai text were meant to be temporary and serve as validation purposes. These could be removed/dropped before merging (or we could just clean up the squash commit message)
BEFORE 🚫
action button

action group

AFTER ✅
action button

action group

How and where has this been tested?
Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.
I tested the deploy preview in AssistivLabs, for WHCM in Chrome, Edge, Firefox.
Validation steps
git checkout 35c9809de3fd760ff8f3848f2c031a4164d849a0
) to see the Thai textRegression testing
Validate:
Screenshots
To-do list