Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .changeset/kind-avocados-poke.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
'@primer/react-brand': minor
---

Anti-aliasing is now applied automatically to all `Text` instances _except_ under these conditions:

- When explicitly disabled via `antialiasing={false}`
- When font weight is `light` or `extralight` AND size is `'100'` or `'200'`
- When size is `100` (regardless of weight)
5 changes: 5 additions & 0 deletions packages/react/src/Accordion/Accordion.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,11 @@ details[open] > .Accordion__summary.Accordion__summary--default .Accordion__summ
line-height: var(--brand-text-lineHeight-200);
}

[data-color-mode='dark'] .Accordion__content {
-webkit-font-smoothing: antialiased;
-moz-osx-font-smoothing: auto;
}

.Accordion__content p:not(:first-child) {
margin-block-start: var(--base-size-16);
}
Expand Down
5 changes: 5 additions & 0 deletions packages/react/src/InlineLink/InlineLink.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,8 @@
.InlineLink:active > span {
color: var(--brand-InlineLink-color-pressed);
}

[data-color-mode='dark'] .InlineLink {
-webkit-font-smoothing: subpixel-antialiased;
Copy link
Contributor

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?

-moz-osx-font-smoothing: auto;
}
5 changes: 5 additions & 0 deletions packages/react/src/Label/Label.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@
text-align: center;
}

[data-color-mode='dark'] .Label {
-webkit-font-smoothing: antialiased;
-moz-osx-font-smoothing: auto;
}

.Label::before {
content: '';
position: absolute;
Expand Down
2 changes: 2 additions & 0 deletions packages/react/src/Prose/Prose.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
/* ---------------------------------------------------------- */

.Prose * + * {
-webkit-font-smoothing: antialiased;
-moz-osx-font-smoothing: auto;
margin-block-start: var(--spacing, 1em);
}

Expand Down
5 changes: 5 additions & 0 deletions packages/react/src/SubNav/SubNav.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@
z-index: 1;
}

[data-color-mode='dark'] .SubNav {
-webkit-font-smoothing: antialiased;
-moz-osx-font-smoothing: grayscale;
}

.SubNav__heading {
font-weight: var(--base-text-weight-semibold);
color: var(--brand-color-text-default);
Expand Down
5 changes: 5 additions & 0 deletions packages/react/src/Text/Text.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@
font-family: var(--brand-body-fontFamily);
}

[data-color-mode='dark'] .Text--antialiased {
-webkit-font-smoothing: antialiased;
-moz-osx-font-smoothing: auto;
}

.Text--default {
color: var(--brand-color-text-default);
}
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/Text/Text.module.css.d.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
declare const styles: {
readonly "Text": string;
readonly "Text--antialiased": string;
readonly "Text--default": string;
readonly "Text--muted": string;
readonly "Text-font--mona-sans": string;
Expand Down
19 changes: 18 additions & 1 deletion packages/react/src/Text/Text.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {Meta, StoryFn} from '@storybook/react'
import React from 'react'
import {Text, TextFontVariants, TextSizes, TextWeights} from '.'
import {Stack, Box, Grid} from '../'
import {Stack, Box, Grid, ThemeProvider} from '../'

import styles from './Text.stories.module.css'

Expand Down Expand Up @@ -43,6 +43,23 @@ Playground.args = {
size: '200',
}

export const AntiAliasingOff = () => (
<ThemeProvider colorMode="dark">
<Box padding="spacious" backgroundColor="default">
<Text as="p" size="200" weight="light">
Anti aliasing is disabled for light text at 16px or smaller on a dark backgrounds.
</Text>
<Text as="p" size="200">
Anti aliasing is enabled for medium text at 16px or larger on a dark backgrounds.
</Text>
</Box>
</ThemeProvider>
)
AntiAliasingOff.args = {
size: '200',
weight: 'light',
}

export const Scale: StoryFn<typeof Text> = args => (
<>
{TextSizes.map(size => (
Expand Down
48 changes: 48 additions & 0 deletions packages/react/src/Text/Text.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -160,4 +160,52 @@ describe('Text', () => {
}
}
})
it('should not apply antialiasing when disabled', () => {
const mockId = 'mock-id'
const {getByText} = render(
<Text data-testid={mockId} weight="light" antialiasing={false}>
AA disabled
</Text>,
)
const el = getByText('AA disabled')
expect(el).not.toHaveClass('Text--antialiased')
})

it('should not apply antialiasing for light weight and small sizes', () => {
const {getByText} = render(
<>
<Text weight="light" size="100">
light 100
</Text>
<Text weight="light" size="200">
light 200
</Text>
<Text weight="extralight" size="100">
extralight 100
</Text>
<Text weight="extralight" size="200">
extralight 200
</Text>
</>,
)

expect(getByText('light 100')).not.toHaveClass('Text--antialiased')
expect(getByText('light 200')).not.toHaveClass('Text--antialiased')
expect(getByText('extralight 100')).not.toHaveClass('Text--antialiased')
expect(getByText('extralight 200')).not.toHaveClass('Text--antialiased')
})

it('should not apply antialiasing for size 100', () => {
const {getByText} = render(<Text size="100">size 100</Text>)
expect(getByText('size 100')).not.toHaveClass('Text--antialiased')
})

it('should apply antialiasing by default for other combinations', () => {
const {getByText} = render(
<Text size="300" weight="medium">
medium 300
</Text>,
)
expect(getByText('medium 300')).toHaveClass('Text--antialiased')
})
})
15 changes: 15 additions & 0 deletions packages/react/src/Text/Text.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ export type TextProps = {
* Note: Only applies to block elements.
*/
align?: 'start' | 'center' | 'end'
/**
* Toggle antialiasing on or off
*/
antialiasing?: boolean
} & TextTags

export function Text({
Expand All @@ -80,6 +84,7 @@ export function Text({
variant = defaultTextVariant,
weight,
style,
antialiasing = true,
Copy link
Contributor

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?

...rest
}: PropsWithChildren<TextProps>) {
const {classes: animationClasses, styles: animationInlineStyles} = useAnimation(animate)
Expand All @@ -96,12 +101,22 @@ export function Text({
.join(' ')
}, [weight])

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',
)

Comment on lines +104 to +112
Copy link
Contributor

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

Suggested change
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)

const textClassName = clsx(
animationClasses,
styles.Text,
styles[`Text-font--${font}`],
styles[`Text--${variant}`],
styles[`Text--${size}`],
!dontApplyAA && styles['Text--antialiased'],
weight && weightClass,
align && styles[`Text-align--${align}`],
className,
Expand Down
7 changes: 7 additions & 0 deletions packages/react/src/Text/Text.visual.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ test.describe('Visual Comparison: Text', () => {
expect(await page.screenshot({fullPage: true})).toMatchSnapshot()
})

test('Text / Anti Aliasing Off', async ({page}) => {
await page.goto('http://localhost:6006/iframe.html?args=&id=components-text--anti-aliasing-off&viewMode=story')

await page.waitForTimeout(500)
expect(await page.screenshot({fullPage: true})).toMatchSnapshot()
})

test('Text / Scale', async ({page}) => {
await page.goto('http://localhost:6006/iframe.html?args=&id=components-text--scale&viewMode=story')

Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading