Skip to content

Support link underline preference #3848

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Support link underline preference #3848

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9031911
Support link underline preference (#3720)
mperrotti Oct 7, 2023
08f8153
Merge branch 'main' into feat/add-link-underline-preference-setting
joshblack Oct 20, 2023
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
7 changes: 7 additions & 0 deletions .changeset/happy-rivers-attend.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@primer/react": minor
---

Updates link styles to support underline link preferences.

<!-- Changed components: ActionList, BranchName, Breadcrumbs, Button, Button2, Heading, Link -->
Binary file added ...ranchName.test.ts-snapshots/BranchName-Default-light-forcedUnderlines-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...adcrumbs.test.ts-snapshots/Breadcrumbs-Default-light-forcedUnderlines-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...components/Link.test.ts-snapshots/Link-Default-light-forcedUnderlines-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...s/components/Link.test.ts-snapshots/Link-Muted-light-forcedUnderlines-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...mponents/Link.test.ts-snapshots/Link-Underline-light-forcedUnderlines-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...kButton.test.ts-snapshots/LinkButton-Invisible-light-forcedUnderlines-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
14 changes: 14 additions & 0 deletions e2e/components/BranchName.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,5 +39,19 @@ test.describe('BranchName', () => {
})
})
}

test.describe('with forced underlines', () => {
test('default @vrt', async ({page}) => {
await visit(page, {
id: 'components-branchname--default',
globals: {
colorScheme: 'light',
prefersLinkUnderlines: 'true',
},
})

expect(await page.screenshot()).toMatchSnapshot('BranchName.Default.light.forcedUnderlines.png')
})
})
})
})
14 changes: 14 additions & 0 deletions e2e/components/Breadcrumbs.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,5 +43,19 @@ test.describe('Breadcrumbs', () => {
})
})
}

test.describe('with forced underlines', () => {
test('default @vrt', async ({page}) => {
await visit(page, {
id: 'components-breadcrumbs--default',
globals: {
colorScheme: 'light',
prefersLinkUnderlines: 'true',
},
})

expect(await page.screenshot()).toMatchSnapshot('Breadcrumbs.Default.light.forcedUnderlines.png')
})
})
})
})
42 changes: 42 additions & 0 deletions e2e/components/Link.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,20 @@ test.describe('Link', () => {
})
})
}

test.describe('with forced underlines', () => {
test('default @vrt', async ({page}) => {
await visit(page, {
id: 'components-link--default',
globals: {
colorScheme: 'light',
prefersLinkUnderlines: 'true',
},
})

expect(await page.screenshot()).toMatchSnapshot('Link.Default.light.forcedUnderlines.png')
})
})
})

test.describe('Muted', () => {
Expand Down Expand Up @@ -85,6 +99,20 @@ test.describe('Link', () => {
})
})
}

test.describe('with forced underlines', () => {
test('default @vrt', async ({page}) => {
await visit(page, {
id: 'components-link-features--muted',
globals: {
colorScheme: 'light',
prefersLinkUnderlines: 'true',
},
})

expect(await page.screenshot()).toMatchSnapshot('Link.Muted.light.forcedUnderlines.png')
})
})
})

test.describe('Underline', () => {
Expand Down Expand Up @@ -127,5 +155,19 @@ test.describe('Link', () => {
})
})
}

test.describe('with forced underlines', () => {
test('default @vrt', async ({page}) => {
await visit(page, {
id: 'components-link-features--underline',
globals: {
colorScheme: 'light',
prefersLinkUnderlines: 'true',
},
})

expect(await page.screenshot()).toMatchSnapshot('Link.Underline.light.forcedUnderlines.png')
})
})
})
})
14 changes: 14 additions & 0 deletions e2e/components/LinkButton.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,20 @@ test.describe('LinkButton', () => {
})
})
}

test.describe('with forced underlines', () => {
test('default @vrt', async ({page}) => {
await visit(page, {
id: 'components-linkbutton-features--invisible',
globals: {
colorScheme: 'light',
prefersLinkUnderlines: 'true',
},
})

expect(await page.screenshot()).toMatchSnapshot('LinkButton.Invisible.light.forcedUnderlines.png')
})
})
})

test.describe('Large', () => {
Expand Down
2 changes: 1 addition & 1 deletion e2e/test-helpers/storybook.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export async function visit(page: Page, options: Options) {
let params = ''
for (const [key, value] of Object.entries(globals)) {
if (params !== '') {
params += '&'
params += ';'
}
params += `${key}:${value}`
}
Expand Down
1 change: 1 addition & 0 deletions src/ActionList/LinkItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export const LinkItem = React.forwardRef(({sx = {}, active, as: Component, ...pr

// inherit Item styles
color: 'inherit',
textDecoration: 'none',
'&:hover': {color: 'inherit', textDecoration: 'none'},
}

Expand Down
7 changes: 7 additions & 0 deletions src/BaseStyles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ const GlobalStyle = createGlobalStyle<{colorScheme?: 'light' | 'dark'}>`
details-dialog:focus:not(:focus-visible):not(.focus-visible) {
outline: none;
}

/* Used to fake conditional styles using a technique by Lea Verou: https://lea.verou.me/blog/2020/10/the-var-space-hack-to-toggle-multiple-values-with-one-custom-property/ */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

omg so clever!

/* We have to use a zero-width space character (\u200B) as the value instead of a regular whitespace character because styled-components strips out properties that just have a whitespace value. */
:root {--prefers-link-underlines: \u200B;}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool hack! But we're only using this for one property: text-decoration so I'm not sure if we should prefer the explicit none here?

[data-a11y-link-underlines='true'] {
--prefers-link-underlines: initial;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think of using the off/on variables like Lea suggested in her blogpost for a clearer understanding of this variable assignment?

}
`

const Base = styled.div<SystemTypographyProps & SystemCommonProps>`
Expand Down
2 changes: 1 addition & 1 deletion src/BranchName/BranchName.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const BranchName = styled.a<SxProp>`
color: ${get('colors.accent.fg')};
background-color: ${get('colors.accent.subtle')};
border-radius: ${get('radii.2')};
text-decoration: none;
text-decoration: var(--prefers-link-underlines, underline);

${sx};
`
Expand Down
4 changes: 2 additions & 2 deletions src/BranchName/__tests__/__snapshots__/BranchName.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ exports[`BranchName renders consistently 1`] = `
color: #0969da;
background-color: #ddf4ff;
border-radius: 6px;
-webkit-text-decoration: none;
text-decoration: none;
-webkit-text-decoration: var(--prefers-link-underlines,underline);
text-decoration: var(--prefers-link-underlines,underline);
}

<a
Expand Down
2 changes: 1 addition & 1 deletion src/Breadcrumbs/Breadcrumbs.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ const BreadcrumbsItem = styled.a.attrs<StyledBreadcrumbsItemProps>(props => ({
color: ${get('colors.accent.fg')};
display: inline-block;
font-size: ${get('fontSizes.1')};
text-decoration: none;
text-decoration: var(--prefers-link-underlines, underline);
&:hover {
text-decoration: underline;
}
Expand Down
12 changes: 6 additions & 6 deletions src/Breadcrumbs/__tests__/__snapshots__/BreadcrumbsItem.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ exports[`Breadcrumbs.Item adds activeClassName={SELECTED_CLASS} when it gets a "
color: #0969da;
display: inline-block;
font-size: 14px;
-webkit-text-decoration: none;
text-decoration: none;
-webkit-text-decoration: var(--prefers-link-underlines,underline);
text-decoration: var(--prefers-link-underlines,underline);
}

.c0:hover {
Expand All @@ -32,8 +32,8 @@ exports[`Breadcrumbs.Item renders consistently 1`] = `
color: #0969da;
display: inline-block;
font-size: 14px;
-webkit-text-decoration: none;
text-decoration: none;
-webkit-text-decoration: var(--prefers-link-underlines,underline);
text-decoration: var(--prefers-link-underlines,underline);
}

.c0:hover {
Expand All @@ -57,8 +57,8 @@ exports[`Breadcrumbs.Item respects the "selected" prop 1`] = `
color: #0969da;
display: inline-block;
font-size: 14px;
-webkit-text-decoration: none;
text-decoration: none;
-webkit-text-decoration: var(--prefers-link-underlines,underline);
text-decoration: var(--prefers-link-underlines,underline);
}

.c0:hover {
Expand Down
6 changes: 4 additions & 2 deletions src/Button/__tests__/__snapshots__/Button.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1512,11 +1512,13 @@ exports[`Button styles invisible button appropriately 1`] = `
display: -webkit-inline-flex;
display: -ms-inline-flexbox;
display: inline-flex;
-webkit-text-decoration: var(--prefers-link-underlines,underline);
text-decoration: var(--prefers-link-underlines,underline);
}

.c0[href]:hover {
-webkit-text-decoration: none;
text-decoration: none;
-webkit-text-decoration: var(--prefers-link-underlines,underline);
text-decoration: var(--prefers-link-underlines,underline);
}

.c0:hover {
Expand Down
6 changes: 6 additions & 0 deletions src/Button/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,12 @@ export const getVariantStyles = (variant: VariantType = 'default', theme?: Theme
backgroundColor: 'transparent',
borderColor: 'transparent',
boxShadow: 'none',
'&[href]': {
textDecoration: 'var(--prefers-link-underlines, underline)',
'&:hover': {
textDecoration: 'var(--prefers-link-underlines, underline)',
},
},
'&:hover:not([disabled])': {
backgroundColor: 'btn.hoverBg',
},
Expand Down
1 change: 1 addition & 0 deletions src/Heading/Heading.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ const Heading = forwardRef(({as: Component = 'h2', ...props}, forwardedRef) => {
return (
<StyledHeading
as={Component}
className="pc-Heading"
{...props}
// @ts-ignore shh
ref={innerRef}
Expand Down
2 changes: 1 addition & 1 deletion src/Heading/__tests__/__snapshots__/Heading.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@ exports[`Heading renders consistently 1`] = `
}

<h2
className="c0"
className="c0 pc-Heading"
/>
`;
4 changes: 2 additions & 2 deletions src/Link/Link.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ const hoverColor = system({

const StyledLink = styled.a<StyledLinkProps>`
color: ${props => (props.muted ? get('colors.fg.muted')(props) : get('colors.accent.fg')(props))};
text-decoration: ${props => (props.underline ? 'underline' : 'none')};
text-decoration: ${props => (props.underline ? 'underline' : 'var(--prefers-link-underlines, underline)')};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming BaseStyles is an optional wrapper... (correct me if it's not!)

Instead of changing the default, should we keep that default and let folks change the default by switching-on --prefers-link-underlines?

text-decoration: props.underline ? 'underline' : 'var(--prefers-link-underlines, none)'

If --prefers-link-underlines is not defined, for example when BaseStyles is not imported, the fallback would be none

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshblack @pksjce Does this sound right or am I missing the goal here?

&:hover {
text-decoration: ${props => (props.muted ? 'none' : 'underline')};
text-decoration: ${props => (props.muted ? 'var(--prefers-link-underlines, underline)' : 'underline')};
${props => (props.hoverColor ? hoverColor : props.muted ? `color: ${get('colors.accent.fg')(props)}` : '')};
}
&:is(button) {
Expand Down
32 changes: 16 additions & 16 deletions src/Link/__tests__/__snapshots__/Link.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
exports[`Link applies button styles when rendering a button element 1`] = `
.c0 {
color: #0969da;
-webkit-text-decoration: none;
text-decoration: none;
-webkit-text-decoration: var(--prefers-link-underlines,underline);
text-decoration: var(--prefers-link-underlines,underline);
}

.c0:hover {
Expand Down Expand Up @@ -37,8 +37,8 @@ exports[`Link applies button styles when rendering a button element 1`] = `
exports[`Link passes href down to link element 1`] = `
.c0 {
color: #0969da;
-webkit-text-decoration: none;
text-decoration: none;
-webkit-text-decoration: var(--prefers-link-underlines,underline);
text-decoration: var(--prefers-link-underlines,underline);
}

.c0:hover {
Expand Down Expand Up @@ -72,8 +72,8 @@ exports[`Link passes href down to link element 1`] = `
exports[`Link renders consistently 1`] = `
.c0 {
color: #0969da;
-webkit-text-decoration: none;
text-decoration: none;
-webkit-text-decoration: var(--prefers-link-underlines,underline);
text-decoration: var(--prefers-link-underlines,underline);
}

.c0:hover {
Expand Down Expand Up @@ -106,8 +106,8 @@ exports[`Link renders consistently 1`] = `
exports[`Link respects hoverColor prop 1`] = `
.c0 {
color: #0969da;
-webkit-text-decoration: none;
text-decoration: none;
-webkit-text-decoration: var(--prefers-link-underlines,underline);
text-decoration: var(--prefers-link-underlines,underline);
}

.c0:hover {
Expand Down Expand Up @@ -141,14 +141,14 @@ exports[`Link respects hoverColor prop 1`] = `
exports[`Link respects the "sx" prop when "muted" prop is also passed 1`] = `
.c0 {
color: #656d76;
-webkit-text-decoration: none;
text-decoration: none;
-webkit-text-decoration: var(--prefers-link-underlines,underline);
text-decoration: var(--prefers-link-underlines,underline);
color: #ffffff;
}

.c0:hover {
-webkit-text-decoration: none;
text-decoration: none;
-webkit-text-decoration: var(--prefers-link-underlines,underline);
text-decoration: var(--prefers-link-underlines,underline);
color: #0969da;
}

Expand Down Expand Up @@ -178,13 +178,13 @@ exports[`Link respects the "sx" prop when "muted" prop is also passed 1`] = `
exports[`Link respects the "muted" prop 1`] = `
.c0 {
color: #656d76;
-webkit-text-decoration: none;
text-decoration: none;
-webkit-text-decoration: var(--prefers-link-underlines,underline);
text-decoration: var(--prefers-link-underlines,underline);
}

.c0:hover {
-webkit-text-decoration: none;
text-decoration: none;
-webkit-text-decoration: var(--prefers-link-underlines,underline);
text-decoration: var(--prefers-link-underlines,underline);
color: #0969da;
}

Expand Down
Loading