From 1a674f7ad18eb51bfc3ea63ec53b14880ebfd25a Mon Sep 17 00:00:00 2001 From: Jon Rohan Date: Wed, 14 Aug 2024 15:03:38 -0700 Subject: [PATCH] Refactor Link component to use CSS modules (#4828) * Refactor Link to use CSS modules * Remove needless disable * Add newline * Create quick-adults-buy.md * Run tests inside fhte feature flag also * Fix naming * Rails interpolation got me * test(e2e): update e2e tests for link (#4825) Co-authored-by: Josh Black * Remove example-nextjs --------- Co-authored-by: Josh Black Co-authored-by: Josh Black --- .changeset/pre.json | 3 +- .changeset/quick-adults-buy.md | 5 ++ .eslintrc.js | 1 - contributor-docs/CONTRIBUTING.md | 46 ++++++------- e2e/components/Link.test.ts | 36 ++++++++++ examples/nextjs/.gitignore | 1 - examples/nextjs/next-env.d.ts | 5 -- examples/nextjs/next.config.js | 5 -- examples/nextjs/package.json | 19 ------ examples/nextjs/src/components/Layout.js | 19 ------ examples/nextjs/src/components/Navigation.js | 69 -------------------- examples/nextjs/src/pages/_app.js | 15 ----- examples/nextjs/src/pages/index.js | 6 -- examples/nextjs/src/pages/tabs/[tab].js | 10 --- examples/nextjs/tsconfig.json | 30 --------- package-lock.json | 26 ++------ packages/react/src/Link/Link.module.css | 40 ++++++++++++ packages/react/src/Link/Link.tsx | 38 ++++++++++- 18 files changed, 148 insertions(+), 226 deletions(-) create mode 100644 .changeset/quick-adults-buy.md delete mode 100644 examples/nextjs/.gitignore delete mode 100644 examples/nextjs/next-env.d.ts delete mode 100644 examples/nextjs/next.config.js delete mode 100644 examples/nextjs/package.json delete mode 100644 examples/nextjs/src/components/Layout.js delete mode 100644 examples/nextjs/src/components/Navigation.js delete mode 100644 examples/nextjs/src/pages/_app.js delete mode 100644 examples/nextjs/src/pages/index.js delete mode 100644 examples/nextjs/src/pages/tabs/[tab].js delete mode 100644 examples/nextjs/tsconfig.json create mode 100644 packages/react/src/Link/Link.module.css diff --git a/.changeset/pre.json b/.changeset/pre.json index 04b9871b2c9..474bad5d717 100644 --- a/.changeset/pre.json +++ b/.changeset/pre.json @@ -6,7 +6,6 @@ "codesandbox": "0.0.0", "example-app-router": "0.0.0", "example-consumer-test": "0.0.0", - "example-nextjs": "0.0.0", "@primer/react": "36.27.0", "rollup-plugin-import-css": "0.0.0", "postcss-preset-primer": "0.0.0" @@ -28,4 +27,4 @@ "twelve-tables-leave", "young-meals-worry" ] -} +} \ No newline at end of file diff --git a/.changeset/quick-adults-buy.md b/.changeset/quick-adults-buy.md new file mode 100644 index 00000000000..281f4551a6e --- /dev/null +++ b/.changeset/quick-adults-buy.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +Refactor Link component to use CSS modules using the feature flag `primer_react_css_modules` diff --git a/.eslintrc.js b/.eslintrc.js index 77286a7ae85..6846aad47d8 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -42,7 +42,6 @@ module.exports = { 'types/**/*', 'consumer-test/**/*', 'contributor-docs/adrs/*', - 'examples/nextjs/**', 'examples/codesandbox/**', // Note: this file is inlined from an external dependency 'packages/react/src/utils/polymorphic.ts', diff --git a/contributor-docs/CONTRIBUTING.md b/contributor-docs/CONTRIBUTING.md index 7797612c5e3..d1269a43102 100644 --- a/contributor-docs/CONTRIBUTING.md +++ b/contributor-docs/CONTRIBUTING.md @@ -1,25 +1,29 @@ # Contribution guidelines -1. [Roadmap](#roadmap) -2. [Before Getting Started](#before-getting-started) -3. [Discussing non-public features or products](#discussing-non-public-features-or-products) -4. [Developing Components](#developing-components) - - [Tools we use](#tools-we-use) - - [File Structure](#file-structure) - - [Component patterns](#component-patterns) - - [SSR compatibility](#ssr-compatibility) - - [Adding the sx prop](#adding-the-sx-prop) - - [Linting](#linting) - - [TypeScript support](#typescript-support) - - [Additional resources](#additional-resources) -5. [Writing documentation](#writing-documentation) -6. [Creating a pull request](#creating-a-pull-request) - - [Adding changeset to your pull request](#adding-changeset-to-your-pull-request) - - [What to expect after opening a pull request](#what-to-expect-after-opening-a-pull-request) - - [What we look for in reviews](#what-we-look-for-in-reviews) - - [Previewing your changes](#previewing-your-changes) -7. [Deploying](#deploying) -8. [Troubleshooting](#troubleshooting) +- [Contribution guidelines](#contribution-guidelines) + - [Roadmap](#roadmap) + - [Before Getting Started](#before-getting-started) + - [Proposing new components](#proposing-new-components) + - [Discussing non-public features or products](#discussing-non-public-features-or-products) + - [Developing components](#developing-components) + - [Tools we use](#tools-we-use) + - [File structure](#file-structure) + - [Component patterns](#component-patterns) + - [SSR compatibility](#ssr-compatibility) + - [Adding the `sx` prop](#adding-the-sx-prop) + - [Linting](#linting) + - [ESLint](#eslint) + - [Markdownlint](#markdownlint) + - [TypeScript support](#typescript-support) + - [Additional resources](#additional-resources) + - [Writing documentation](#writing-documentation) + - [Creating a pull request](#creating-a-pull-request) + - [Adding changeset to your pull request](#adding-changeset-to-your-pull-request) + - [What to expect after opening a pull request](#what-to-expect-after-opening-a-pull-request) + - [What we look for in reviews](#what-we-look-for-in-reviews) + - [Previewing your changes](#previewing-your-changes) + - [Deploying](#deploying) + - [Troubleshooting](#troubleshooting) ## Roadmap @@ -148,8 +152,6 @@ We consider a component SSR-compatible if it... We use [`eslint-plugin-ssr-friendly`](https://github.com/kopiro/eslint-plugin-ssr-friendly) to prevent misuse of DOM globals. If you see an error from this plugin, please fix it before merging your PR. -If your component doesn't use DOM globals, it likely won't cause layout shift during hydration. However, if you suspect that your component might cause layout shift, you can use the example Next.js app (`examples/nextjs`) to debug. Import and render your component in `examples/nextjs/src/pages/index.js` then run the example app with `cd examples/nextjs && npm run develop`. - ### Adding the `sx` prop Each component should accept a prop called `sx` that allows for setting theme-aware ad-hoc styles. See the [overriding styles](https://primer.style/react/overriding-styles) doc for more information on using the prop. diff --git a/e2e/components/Link.test.ts b/e2e/components/Link.test.ts index 692444882f2..dc1f2f60d5c 100644 --- a/e2e/components/Link.test.ts +++ b/e2e/components/Link.test.ts @@ -31,6 +31,9 @@ test.describe('Link', () => { id: story.id, globals: { colorScheme: theme, + featureFlags: { + primer_react_css_modules: true, + }, }, }) @@ -47,6 +50,39 @@ test.describe('Link', () => { }) test('axe @aat', async ({page}) => { + await visit(page, { + id: story.id, + globals: { + colorScheme: theme, + featureFlags: { + primer_react_css_modules: true, + }, + }, + }) + await expect(page).toHaveNoViolations() + }) + + test('default (styled-component) @vrt', async ({page}) => { + await visit(page, { + id: story.id, + globals: { + colorScheme: theme, + }, + }) + + // Default state + expect(await page.screenshot()).toMatchSnapshot(`Link.${story.title}.${theme}.png`) + + // Hover state + await page.getByRole('link').hover() + expect(await page.screenshot()).toMatchSnapshot(`Link.${story.title}.${theme}.hover.png`) + + // Focus state + await page.keyboard.press('Tab') + expect(await page.screenshot()).toMatchSnapshot(`Link.${story.title}.${theme}.focus.png`) + }) + + test('axe (styled-component) @aat', async ({page}) => { await visit(page, { id: story.id, globals: { diff --git a/examples/nextjs/.gitignore b/examples/nextjs/.gitignore deleted file mode 100644 index a680367ef56..00000000000 --- a/examples/nextjs/.gitignore +++ /dev/null @@ -1 +0,0 @@ -.next diff --git a/examples/nextjs/next-env.d.ts b/examples/nextjs/next-env.d.ts deleted file mode 100644 index 4f11a03dc6c..00000000000 --- a/examples/nextjs/next-env.d.ts +++ /dev/null @@ -1,5 +0,0 @@ -/// -/// - -// NOTE: This file should not be edited -// see https://nextjs.org/docs/basic-features/typescript for more information. diff --git a/examples/nextjs/next.config.js b/examples/nextjs/next.config.js deleted file mode 100644 index 89bfa829544..00000000000 --- a/examples/nextjs/next.config.js +++ /dev/null @@ -1,5 +0,0 @@ -module.exports = { - experimental: { - esmExternals: 'loose', - }, -} diff --git a/examples/nextjs/package.json b/examples/nextjs/package.json deleted file mode 100644 index 62a3b3563b5..00000000000 --- a/examples/nextjs/package.json +++ /dev/null @@ -1,19 +0,0 @@ -{ - "name": "example-nextjs", - "private": true, - "version": "0.0.0", - "scripts": { - "build": "next build", - "develop": "next", - "start": "next start", - "type-check": "tsc --noEmit" - }, - "dependencies": { - "@primer/octicons-react": "19.x", - "@primer/react": "37.0.0-rc.1", - "next": "^14.1.0", - "react": "^18.3.1", - "react-dom": "^18.3.1", - "styled-components": "5.x" - } -} diff --git a/examples/nextjs/src/components/Layout.js b/examples/nextjs/src/components/Layout.js deleted file mode 100644 index 3f32ecb513b..00000000000 --- a/examples/nextjs/src/components/Layout.js +++ /dev/null @@ -1,19 +0,0 @@ -import React from 'react' -import Navigation from './Navigation' -import {Box} from '@primer/react' - -export default function Layout({children}) { - return ( - // - // - - <> - -
{children}
- - - // - // content - // - ) -} diff --git a/examples/nextjs/src/components/Navigation.js b/examples/nextjs/src/components/Navigation.js deleted file mode 100644 index dcc76073652..00000000000 --- a/examples/nextjs/src/components/Navigation.js +++ /dev/null @@ -1,69 +0,0 @@ -import {Box} from '@primer/react' -import { - CodeIcon, - IssueOpenedIcon, - GitPullRequestIcon, - CommentDiscussionIcon, - PlayIcon, - ProjectIcon, - GraphIcon, - GearIcon, - ShieldLockIcon, -} from '@primer/octicons-react' -import React from 'react' -import {UnderlineNav} from '@primer/react' -import {useRouter} from 'next/router' -import Link from 'next/link' - -export default function Navigation() { - const items = [ - {navigation: 'Code', href: '/tabs/code', icon: CodeIcon}, - { - navigation: 'Issues', - href: '/tabs/issues', - icon: IssueOpenedIcon, - counter: 120, - }, - { - navigation: 'Pull Requests', - href: '/tabs/pull-requests', - icon: GitPullRequestIcon, - counter: 13, - }, - { - navigation: 'Discussions', - href: '/tabs/discussions', - icon: CommentDiscussionIcon, - counter: 5, - }, - {navigation: 'Actions', href: '/tabs/actions', icon: PlayIcon, counter: 4}, - { - navigation: 'Projects', - href: '/tabs/projects', - icon: ProjectIcon, - counter: 9, - }, - {navigation: 'Insights', href: '/tabs/insights', icon: GraphIcon}, - {navigation: 'Settings', href: '/tabs/settings', icon: GearIcon, counter: 10}, - {navigation: 'Security', href: '/tabs/security', icon: ShieldLockIcon}, - ] - return ( - - {items.map(item => ( - - {item.navigation} - - ))} - - ) -} - -function UnderlineNavItem({href, children, ...rest}) { - const router = useRouter() - const isCurrent = typeof href === 'string' ? router.asPath === href : router.pathname === href.pathname - return ( - - {children} - - ) -} diff --git a/examples/nextjs/src/pages/_app.js b/examples/nextjs/src/pages/_app.js deleted file mode 100644 index cf4a3e76266..00000000000 --- a/examples/nextjs/src/pages/_app.js +++ /dev/null @@ -1,15 +0,0 @@ -import {ThemeProvider, BaseStyles} from '@primer/react' -import React from 'react' -import Layout from '../components/Layout' - -export default function App({Component, pageProps}) { - return ( - - - - - - - - ) -} diff --git a/examples/nextjs/src/pages/index.js b/examples/nextjs/src/pages/index.js deleted file mode 100644 index 0e9dc75456a..00000000000 --- a/examples/nextjs/src/pages/index.js +++ /dev/null @@ -1,6 +0,0 @@ -import {Box} from '@primer/react' -import React from 'react' - -export default function IndexPage() { - return Welcome to Primer React's Next.js Kitchen Sink -} diff --git a/examples/nextjs/src/pages/tabs/[tab].js b/examples/nextjs/src/pages/tabs/[tab].js deleted file mode 100644 index a4d06d37efe..00000000000 --- a/examples/nextjs/src/pages/tabs/[tab].js +++ /dev/null @@ -1,10 +0,0 @@ -import {useRouter} from 'next/router' - -const Tabs = () => { - const router = useRouter() - const {tab} = router.query - - return

Tab: {tab}

-} - -export default Tabs diff --git a/examples/nextjs/tsconfig.json b/examples/nextjs/tsconfig.json deleted file mode 100644 index 7455936a76c..00000000000 --- a/examples/nextjs/tsconfig.json +++ /dev/null @@ -1,30 +0,0 @@ -{ - "compilerOptions": { - "target": "es5", - "lib": [ - "dom", - "dom.iterable", - "esnext" - ], - "allowJs": true, - "skipLibCheck": true, - "strict": false, - "forceConsistentCasingInFileNames": true, - "noEmit": true, - "incremental": true, - "esModuleInterop": true, - "module": "esnext", - "moduleResolution": "bundler", - "resolveJsonModule": true, - "isolatedModules": true, - "jsx": "preserve" - }, - "include": [ - "next-env.d.ts", - "**/*.ts", - "**/*.tsx" - ], - "exclude": [ - "node_modules" - ] -} diff --git a/package-lock.json b/package-lock.json index 79399125e11..0904d9bc007 100644 --- a/package-lock.json +++ b/package-lock.json @@ -292,7 +292,7 @@ "name": "example-app-router", "version": "0.0.0", "dependencies": { - "@primer/react": "37.0.0-rc.0", + "@primer/react": "37.0.0-rc.1", "next": "^14.1.0", "react": "^18.3.1", "react-dom": "^18.3.1", @@ -311,7 +311,7 @@ "react-dom": "^18.3.1" }, "devDependencies": { - "@primer/react": "37.0.0-rc.0", + "@primer/react": "37.0.0-rc.1", "@types/react": "^18.3.3", "@types/react-dom": "^18.3.0", "@typescript-eslint/eslint-plugin": "^7.11.0", @@ -329,7 +329,7 @@ "name": "example-consumer-test", "version": "0.0.0", "dependencies": { - "@primer/react": "37.0.0-rc.0", + "@primer/react": "37.0.0-rc.1", "@types/react": "^18.2.14", "@types/react-dom": "^18.2.19", "@types/styled-components": "^5.1.11", @@ -350,18 +350,6 @@ "node": ">=4.2.0" } }, - "examples/nextjs": { - "name": "example-nextjs", - "version": "0.0.0", - "dependencies": { - "@primer/octicons-react": "19.x", - "@primer/react": "37.0.0-rc.0", - "next": "^14.1.0", - "react": "^18.3.1", - "react-dom": "^18.3.1", - "styled-components": "5.x" - } - }, "node_modules/@aashutoshrathi/word-wrap": { "version": "1.2.6", "license": "MIT", @@ -30907,10 +30895,6 @@ "resolved": "examples/consumer-test", "link": true }, - "node_modules/example-nextjs": { - "resolved": "examples/nextjs", - "link": true - }, "node_modules/exec-sh": { "version": "0.3.6", "license": "MIT" @@ -62418,7 +62402,7 @@ }, "packages/react": { "name": "@primer/react", - "version": "37.0.0-rc.0", + "version": "37.0.0-rc.1", "license": "MIT", "dependencies": { "@github/combobox-nav": "^2.1.5", @@ -63219,4 +63203,4 @@ } } } -} +} \ No newline at end of file diff --git a/packages/react/src/Link/Link.module.css b/packages/react/src/Link/Link.module.css new file mode 100644 index 00000000000..6fe46242c04 --- /dev/null +++ b/packages/react/src/Link/Link.module.css @@ -0,0 +1,40 @@ +.Link { + color: var(--fgColor-accent); + text-decoration: none; + + /* Reset for button tags */ + &:is(button) { + display: inline-block; + padding: 0; + font-size: inherit; + white-space: nowrap; + cursor: pointer; + user-select: none; + background-color: transparent; + border: 0; + appearance: none; + } + + &:hover { + text-decoration: underline; + } + + /* Deprecated: but need to support backwards compatibility */ + &[data-underline='true'], + /* + Inline links (inside a text block), however, should have underline based on accessibility setting set in data-attribute + Note: setting underline={false} does not override this + */ + [data-a11y-link-underlines='true'] &[data-inline='true'] { + text-decoration: underline; + } + + &[data-muted='true'] { + color: var(--fgColor-muted); + + &:hover { + color: var(--fgColor-accent); + text-decoration: none; + } + } +} diff --git a/packages/react/src/Link/Link.tsx b/packages/react/src/Link/Link.tsx index 36366762dbd..cdc7743addb 100644 --- a/packages/react/src/Link/Link.tsx +++ b/packages/react/src/Link/Link.tsx @@ -1,3 +1,4 @@ +import cx from 'clsx' import React, {forwardRef, useEffect} from 'react' import styled from 'styled-components' import {system} from 'styled-system' @@ -5,6 +6,9 @@ import {get} from '../constants' import {useRefObjectAsForwardedRef} from '../hooks' import type {SxProp} from '../sx' import sx from '../sx' +import classes from './Link.module.css' +import {useFeatureFlag} from '../FeatureFlags' +import Box from '../Box' import type {ComponentProps} from '../utils/types' import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' @@ -57,7 +61,9 @@ const StyledLink = styled.a` ${sx}; ` -const Link = forwardRef(({as: Component = 'a', ...props}, forwardedRef) => { +const Link = forwardRef(({as: Component = 'a', className, ...props}, forwardedRef) => { + const enabled = useFeatureFlag('primer_react_css_modules') + const innerRef = React.useRef(null) useRefObjectAsForwardedRef(forwardedRef, innerRef) @@ -85,9 +91,39 @@ const Link = forwardRef(({as: Component = 'a', ...props}, forwardedRef) => { }, [innerRef]) } + if (enabled) { + if (props.sx) { + return ( + + ) + } + + return ( + + ) + } + return (