-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add anti-aliasing to Text and other components #884
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 0a59583 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
🟢 No design token changes found |
🟢 No visual differences foundOur visual comparison tests did not find any differences in the UI. |
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.
Looks good to me, no concerns. Thank you for raising the accessibility concerns as well.
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.
Looking good! Some small thoughts but nothing blocking
@@ -23,3 +23,8 @@ | |||
.InlineLink:active > span { | |||
color: var(--brand-InlineLink-color-pressed); | |||
} | |||
|
|||
[data-color-mode='dark'] .InlineLink { | |||
-webkit-font-smoothing: subpixel-antialiased; |
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.
Is there a reason that this uses subpixel-antialiased
where others just user antialiased
?
const dontApplyAA = Boolean( | ||
// When explicitly disabled | ||
!antialiasing || | ||
// When selected font weight is not suitable for anti-aliasing | ||
(weight && ['light', 'extralight'].includes(weight as TextWeightVariants) && ['100', '200'].includes(size)) || | ||
// When size is too small | ||
size === '100', | ||
) | ||
|
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 logic could be simplified slightly by bringing the size === '100'
check forward.
Just a matter of opinion, but also extracting the font-weight check into a variable could do away for the need for explicit comments too. Up to you though
const dontApplyAA = Boolean( | |
// When explicitly disabled | |
!antialiasing || | |
// When selected font weight is not suitable for anti-aliasing | |
(weight && ['light', 'extralight'].includes(weight as TextWeightVariants) && ['100', '200'].includes(size)) || | |
// When size is too small | |
size === '100', | |
) | |
const isLighterFontWeight = ['light', 'extralight'].includes(weight as TextWeightVariants) | |
const dontApplyAA = !antialiasing || size === '100' || (size === '200' && isLighterFontWeight) |
@@ -80,6 +84,7 @@ export function Text({ | |||
variant = defaultTextVariant, | |||
weight, | |||
style, | |||
antialiasing = true, |
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.
Minor thing, but I wonder if this would be clearer if it were named antialiasingEnabled
or something similar to more clearly indicate that it's a boolean prop.
wdyt?
Summary
Closes #871
Part of https://github.com/github/primer/issues/4601
Follow up to #482
Adds targeted
font-smooth
toText
and other components in dark mode.Important
Applying blanket AA on websites using font-smooth is NOT recommended due to missing web standards
It's also not recommended to use it anywhere except dark mode.
We will only apply AA when:
light
orextralight
variants, due to poor legibility16px
List of notable changes:
What should reviewers focus on?
Steps to test:
Contributor checklist:
update snapshots
label to the PR)Reviewer checklist:
Screenshots: