From bfe1252587f33e05b699334292a1cd4bacd66d25 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Wed, 15 Mar 2023 13:20:44 -0600 Subject: [PATCH 1/9] Use SSR-compatible slot implementation in PageLayout --- examples/nextjs/package-lock.json | 94 +++--- src/PageLayout/PageLayout.tsx | 95 +++---- .../__snapshots__/PageLayout.test.tsx.snap | 268 +++++++++--------- .../SplitPageLayout.test.tsx.snap | 110 +++---- src/hooks/unstable_useSlots.ts | 59 ++++ 5 files changed, 341 insertions(+), 285 deletions(-) create mode 100644 src/hooks/unstable_useSlots.ts diff --git a/examples/nextjs/package-lock.json b/examples/nextjs/package-lock.json index 425e6fee9bc..accbf490f2e 100644 --- a/examples/nextjs/package-lock.json +++ b/examples/nextjs/package-lock.json @@ -17,7 +17,7 @@ }, "../..": { "name": "@primer/react", - "version": "35.20.0", + "version": "35.21.0", "license": "MIT", "dependencies": { "@github/combobox-nav": "^2.1.5", @@ -26,7 +26,7 @@ "@github/relative-time-element": "^4.1.2", "@lit-labs/react": "1.1.1", "@primer/behaviors": "1.3.3", - "@primer/octicons-react": "^17.7.0", + "@primer/octicons-react": "^18.0.0", "@primer/primitives": "7.11.1", "@react-aria/ssr": "^3.1.0", "@styled-system/css": "^5.1.5", @@ -47,14 +47,14 @@ }, "devDependencies": { "@actions/core": "1.10.0", - "@babel/cli": "7.19.3", + "@babel/cli": "7.21.0", "@babel/core": "7.21.0", "@babel/eslint-parser": "7.19.1", "@babel/plugin-proposal-nullish-coalescing-operator": "7.18.6", - "@babel/plugin-proposal-optional-chaining": "7.16.7", - "@babel/plugin-transform-modules-commonjs": "7.19.6", + "@babel/plugin-proposal-optional-chaining": "7.21.0", + "@babel/plugin-transform-modules-commonjs": "7.21.2", "@babel/preset-react": "7.18.6", - "@babel/preset-typescript": "7.18.6", + "@babel/preset-typescript": "7.21.0", "@changesets/changelog-github": "0.4.8", "@changesets/cli": "2.26.0", "@github/markdownlint-github": "^0.3.0", @@ -67,7 +67,7 @@ "@rollup/plugin-terser": "0.4.0", "@rollup/plugin-typescript": "11.0.0", "@rollup/plugin-virtual": "3.0.1", - "@size-limit/preset-big-lib": "7.0.3", + "@size-limit/preset-big-lib": "8.2.4", "@storybook/addon-a11y": "6.5.14", "@storybook/addon-actions": "6.5.16", "@storybook/addon-essentials": "6.5.16", @@ -77,7 +77,7 @@ "@storybook/builder-webpack5": "6.5.16", "@storybook/jest": "0.0.10", "@storybook/manager-webpack5": "6.5.14", - "@storybook/react": "6.5.15", + "@storybook/react": "6.5.16", "@storybook/test-runner": "0.9.1", "@storybook/testing-library": "0.0.13", "@storybook/theming": "6.5.16", @@ -94,15 +94,16 @@ "@types/lodash.isobject": "3.0.7", "@types/lodash.keyby": "4.6.7", "@types/node": "16.11.11", - "@types/react": "18.0.26", + "@types/react": "18.0.28", "@types/react-dom": "18.0.9", - "@typescript-eslint/eslint-plugin": "4.33.0", - "@typescript-eslint/parser": "4.33.0", + "@typescript-eslint/eslint-plugin": "5.54.1", + "@typescript-eslint/parser": "5.54.1", "ajv": "8.12.0", "axe-core": "4.6.1", "babel-core": "7.0.0-bridge.0", "babel-loader": "^9.1.0", "babel-plugin-add-react-displayname": "0.0.5", + "babel-plugin-dev-expression": "0.2.3", "babel-plugin-macros": "3.1.0", "babel-plugin-open-source": "1.3.4", "babel-plugin-preval": "5.1.0", @@ -116,29 +117,30 @@ "concurrently": "7.6.0", "copyfiles": "2.4.1", "cross-env": "7.0.3", - "eslint": "7.32.0", - "eslint-plugin-github": "4.1.3", - "eslint-plugin-jest": "24.3.6", - "eslint-plugin-jsx-a11y": "6.6.1", - "eslint-plugin-mdx": "1.15.1", + "eslint": "8.35.0", + "eslint-import-resolver-typescript": "3.5.3", + "eslint-plugin-github": "4.6.1", + "eslint-plugin-jest": "27.2.1", + "eslint-plugin-jsx-a11y": "6.7.1", + "eslint-plugin-mdx": "2.0.5", "eslint-plugin-prettier": "4.2.1", - "eslint-plugin-primer-react": "0.7.4", + "eslint-plugin-primer-react": "1.0.1", "eslint-plugin-react": "7.32.2", "eslint-plugin-react-hooks": "4.6.0", - "eslint-plugin-storybook": "0.6.10", + "eslint-plugin-storybook": "0.6.11", "fast-glob": "3.2.12", "filesize": "10.0.6", "front-matter": "4.0.2", "gzip-size": "6.0.0", "husky": "8.0.2", "jest": "29.4.2", - "jest-axe": "5.0.1", + "jest-axe": "7.0.0", "jest-environment-jsdom": "29.4.3", "jest-fail-on-console": "3.0.2", "jest-matchmedia-mock": "1.1.0", "jest-styled-components": "6.3.4", "jest-watch-typeahead": "2.1.1", - "jscodeshift": "0.13.0", + "jscodeshift": "0.14.0", "lint-staged": "13.1.0", "lodash.groupby": "4.6.0", "lodash.isempty": "4.4.0", @@ -149,7 +151,7 @@ "mdast-util-from-markdown": "1.2.0", "mdast-util-frontmatter": "1.0.1", "mdast-util-mdx": "2.0.1", - "mdast-util-to-string": "3.1.0", + "mdast-util-to-string": "3.1.1", "micromark-extension-frontmatter": "1.0.0", "micromark-extension-mdxjs": "1.0.0", "prettier": "2.8.1", @@ -163,7 +165,7 @@ "rollup": "3.12.1", "rollup-plugin-visualizer": "5.9.0", "semver": "7.3.8", - "size-limit": "7.0.3", + "size-limit": "8.2.4", "storybook-addon-turbo-build": "1.1.0", "styled-components": "4.4.1", "terser": "5.16.1", @@ -173,7 +175,7 @@ "unist-util-find": "1.0.2", "unist-util-find-before": "3.0.0", "unist-util-flat-filter": "2.0.0", - "webpack": "5.74.0", + "webpack": "5.76.0", "yaml": "2.2.1" }, "engines": { @@ -1351,14 +1353,14 @@ "version": "file:../..", "requires": { "@actions/core": "1.10.0", - "@babel/cli": "7.19.3", + "@babel/cli": "7.21.0", "@babel/core": "7.21.0", "@babel/eslint-parser": "7.19.1", "@babel/plugin-proposal-nullish-coalescing-operator": "7.18.6", - "@babel/plugin-proposal-optional-chaining": "7.16.7", - "@babel/plugin-transform-modules-commonjs": "7.19.6", + "@babel/plugin-proposal-optional-chaining": "7.21.0", + "@babel/plugin-transform-modules-commonjs": "7.21.2", "@babel/preset-react": "7.18.6", - "@babel/preset-typescript": "7.18.6", + "@babel/preset-typescript": "7.21.0", "@changesets/changelog-github": "0.4.8", "@changesets/cli": "2.26.0", "@github/combobox-nav": "^2.1.5", @@ -1370,7 +1372,7 @@ "@lit-labs/react": "1.1.1", "@playwright/test": "1.30.0", "@primer/behaviors": "1.3.3", - "@primer/octicons-react": "^17.7.0", + "@primer/octicons-react": "^18.0.0", "@primer/primitives": "7.11.1", "@react-aria/ssr": "^3.1.0", "@rollup/plugin-babel": "6.0.3", @@ -1380,7 +1382,7 @@ "@rollup/plugin-terser": "0.4.0", "@rollup/plugin-typescript": "11.0.0", "@rollup/plugin-virtual": "3.0.1", - "@size-limit/preset-big-lib": "7.0.3", + "@size-limit/preset-big-lib": "8.2.4", "@storybook/addon-a11y": "6.5.14", "@storybook/addon-actions": "6.5.16", "@storybook/addon-essentials": "6.5.16", @@ -1390,7 +1392,7 @@ "@storybook/builder-webpack5": "6.5.16", "@storybook/jest": "0.0.10", "@storybook/manager-webpack5": "6.5.14", - "@storybook/react": "6.5.15", + "@storybook/react": "6.5.16", "@storybook/test-runner": "0.9.1", "@storybook/testing-library": "0.0.13", "@storybook/theming": "6.5.16", @@ -1410,19 +1412,20 @@ "@types/lodash.isobject": "3.0.7", "@types/lodash.keyby": "4.6.7", "@types/node": "16.11.11", - "@types/react": "18.0.26", + "@types/react": "18.0.28", "@types/react-dom": "18.0.9", "@types/styled-components": "^5.1.11", "@types/styled-system": "^5.1.12", "@types/styled-system__css": "^5.0.16", "@types/styled-system__theme-get": "^5.0.1", - "@typescript-eslint/eslint-plugin": "4.33.0", - "@typescript-eslint/parser": "4.33.0", + "@typescript-eslint/eslint-plugin": "5.54.1", + "@typescript-eslint/parser": "5.54.1", "ajv": "8.12.0", "axe-core": "4.6.1", "babel-core": "7.0.0-bridge.0", "babel-loader": "^9.1.0", "babel-plugin-add-react-displayname": "0.0.5", + "babel-plugin-dev-expression": "0.2.3", "babel-plugin-macros": "3.1.0", "babel-plugin-open-source": "1.3.4", "babel-plugin-preval": "5.1.0", @@ -1439,16 +1442,17 @@ "copyfiles": "2.4.1", "cross-env": "7.0.3", "deepmerge": "^4.2.2", - "eslint": "7.32.0", - "eslint-plugin-github": "4.1.3", - "eslint-plugin-jest": "24.3.6", - "eslint-plugin-jsx-a11y": "6.6.1", - "eslint-plugin-mdx": "1.15.1", + "eslint": "8.35.0", + "eslint-import-resolver-typescript": "3.5.3", + "eslint-plugin-github": "4.6.1", + "eslint-plugin-jest": "27.2.1", + "eslint-plugin-jsx-a11y": "6.7.1", + "eslint-plugin-mdx": "2.0.5", "eslint-plugin-prettier": "4.2.1", - "eslint-plugin-primer-react": "0.7.4", + "eslint-plugin-primer-react": "1.0.1", "eslint-plugin-react": "7.32.2", "eslint-plugin-react-hooks": "4.6.0", - "eslint-plugin-storybook": "0.6.10", + "eslint-plugin-storybook": "0.6.11", "fast-glob": "3.2.12", "filesize": "10.0.6", "focus-visible": "^5.2.0", @@ -1458,13 +1462,13 @@ "history": "^5.0.0", "husky": "8.0.2", "jest": "29.4.2", - "jest-axe": "5.0.1", + "jest-axe": "7.0.0", "jest-environment-jsdom": "29.4.3", "jest-fail-on-console": "3.0.2", "jest-matchmedia-mock": "1.1.0", "jest-styled-components": "6.3.4", "jest-watch-typeahead": "2.1.1", - "jscodeshift": "0.13.0", + "jscodeshift": "0.14.0", "lint-staged": "13.1.0", "lodash.groupby": "4.6.0", "lodash.isempty": "4.4.0", @@ -1475,7 +1479,7 @@ "mdast-util-from-markdown": "1.2.0", "mdast-util-frontmatter": "1.0.1", "mdast-util-mdx": "2.0.1", - "mdast-util-to-string": "3.1.0", + "mdast-util-to-string": "3.1.1", "micromark-extension-frontmatter": "1.0.0", "micromark-extension-mdxjs": "1.0.0", "prettier": "2.8.1", @@ -1490,7 +1494,7 @@ "rollup": "3.12.1", "rollup-plugin-visualizer": "5.9.0", "semver": "7.3.8", - "size-limit": "7.0.3", + "size-limit": "8.2.4", "storybook-addon-turbo-build": "1.1.0", "styled-components": "4.4.1", "styled-system": "^5.1.5", @@ -1501,7 +1505,7 @@ "unist-util-find": "1.0.2", "unist-util-find-before": "3.0.0", "unist-util-flat-filter": "2.0.0", - "webpack": "5.74.0", + "webpack": "5.76.0", "yaml": "2.2.1" } }, diff --git a/src/PageLayout/PageLayout.tsx b/src/PageLayout/PageLayout.tsx index f06af5ff006..86ac633c64d 100644 --- a/src/PageLayout/PageLayout.tsx +++ b/src/PageLayout/PageLayout.tsx @@ -1,18 +1,17 @@ import React from 'react' import {createGlobalStyle} from 'styled-components' import Box from '../Box' +// eslint-disable-next-line camelcase +import {unstable_useSlots} from '../hooks/unstable_useSlots' import {useId} from '../hooks/useId' import {useRefObjectAsForwardedRef} from '../hooks/useRefObjectAsForwardedRef' import {isResponsiveValue, ResponsiveValue, useResponsiveValue} from '../hooks/useResponsiveValue' import {BetterSystemStyleObject, merge, SxProp} from '../sx' import {Theme} from '../ThemeProvider' -import createSlots from '../utils/create-slots' import {canUseDOM} from '../utils/environment' import VisuallyHidden from '../_VisuallyHidden' import {useStickyPaneHeight} from './useStickyPaneHeight' -const {Slots, Slot} = createSlots(['Header', 'Footer']) - const REGION_ORDER = { header: 0, paneStart: 1, @@ -71,6 +70,12 @@ const Root: React.FC> = ({ }) => { const {rootRef, enableStickyPane, disableStickyPane, contentTopRef, contentBottomRef, stickyPaneHeight} = useStickyPaneHeight() + + const [slots, rest] = unstable_useSlots(children, { + header: Header, + footer: Footer, + }) + return ( > = ({ flexWrap: 'wrap', }} > - - {slots => { - return ( - <> - {slots.Header} - {children} - {slots.Footer} - - ) - }} - + {slots.header} + {rest} + {slots.footer} @@ -361,24 +358,22 @@ const Header: React.FC> = ({ const isHidden = useResponsiveValue(hidden, false) const {rowGap} = React.useContext(PageLayoutContext) return ( - - - + ) } @@ -826,25 +821,23 @@ const Footer: React.FC> = ({ const isHidden = useResponsiveValue(hidden, false) const {rowGap} = React.useContext(PageLayoutContext) return ( - - - + ) } diff --git a/src/PageLayout/__snapshots__/PageLayout.test.tsx.snap b/src/PageLayout/__snapshots__/PageLayout.test.tsx.snap index 59604aa94a9..97b5bfbea64 100644 --- a/src/PageLayout/__snapshots__/PageLayout.test.tsx.snap +++ b/src/PageLayout/__snapshots__/PageLayout.test.tsx.snap @@ -14,6 +14,10 @@ exports[`PageLayout renders condensed layout 1`] = ` flex-wrap: wrap; } +.c3 { + padding: 0; +} + .c5 { display: -webkit-box; display: -webkit-flex; @@ -72,14 +76,22 @@ exports[`PageLayout renders condensed layout 1`] = ` padding: 0; } -.c3 { - padding: 0; -} - .c0 { padding: 16px; } +.c2 { + width: 100%; + margin-bottom: 16px; +} + +.c4 { + margin-left: -16px; + margin-right: -16px; + display: none; + margin-top: 16px; +} + .c8 { display: -webkit-box; display: -webkit-flex; @@ -111,18 +123,6 @@ exports[`PageLayout renders condensed layout 1`] = ` margin-right: 16px; } -.c2 { - width: 100%; - margin-bottom: 16px; -} - -.c4 { - margin-left: -16px; - margin-right: -16px; - display: none; - margin-top: 16px; -} - .c12 { -webkit-order: 4; -ms-flex-order: 4; @@ -150,6 +150,13 @@ exports[`PageLayout renders condensed layout 1`] = ` } } +@media screen and (min-width:768px) { + .c4 { + margin-left: 0 !important; + margin-right: 0 !important; + } +} + @media screen and (min-width:768px) { .c8 { width: auto; @@ -169,13 +176,6 @@ exports[`PageLayout renders condensed layout 1`] = ` } } -@media screen and (min-width:768px) { - .c4 { - margin-left: 0 !important; - margin-right: 0 !important; - } -} -
-
-
- Header -
-
-
-
+
+ Header +
+
+
@@ -265,19 +265,19 @@ exports[`SplitPageLayout renders default layout 1`] = ` Pane
-
-
-
-
- Footer -
-
+
+
+ Footer +
+ +
diff --git a/src/hooks/unstable_useSlots.ts b/src/hooks/unstable_useSlots.ts new file mode 100644 index 00000000000..ebd6549d3ea --- /dev/null +++ b/src/hooks/unstable_useSlots.ts @@ -0,0 +1,59 @@ +import React from 'react' + +type SlotConfig = Record + +type SlotElements = { + [Property in keyof Type]: React.ReactElement +} + +/** + * Extract components from `children` so we can render them in different places, + * allowing us to implement components with SSR-compatible slot APIs. + * Note: We can only extract direct children, not nested ones. + */ +// eslint-disable-next-line camelcase +export function unstable_useSlots( + children: React.ReactNode, + config: T, +): [Partial>, React.ReactNode[]] { + // Object mapping slot names to their elements + const slots: Partial> = mapValues(config, () => undefined) + + // Array of elements that are not slots + const rest: React.ReactNode[] = [] + + // eslint-disable-next-line github/array-foreach + React.Children.forEach(children, child => { + if (!React.isValidElement(child)) { + rest.push(child) + return + } + + // Check if the child is a slot + const slotKey = getKeyByValue(config, child.type) + + if (slotKey && !slots[slotKey]) { + // If the child is a slot, add it to the `slots` object + slots[slotKey] = child + } else { + // If the child is not a slot, add it to the `rest` array + rest.push(child) + } + }) + + return [slots, rest] +} + +/** Map the values of an object */ +function mapValues, V>(obj: T, fn: (value: T[keyof T]) => V) { + return Object.keys(obj).reduce((result, key: keyof T) => { + result[key] = fn(obj[key]) + return result + }, {} as Record) +} + +/** Get the key of an object by its value */ +function getKeyByValue>(object: T, value: unknown): keyof T | undefined { + const keys = Object.keys(object) as (keyof T)[] + return keys.find(key => object[key] === value) +} From 69b79facc8b986cca6f10d630aee2f584294dafc Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Wed, 15 Mar 2023 14:04:14 -0600 Subject: [PATCH 2/9] Add tests for unstable_useSlots --- .../__tests__/unstable_useSlots.test.tsx | 115 ++++++++++++++++++ src/hooks/unstable_useSlots.ts | 14 ++- 2 files changed, 125 insertions(+), 4 deletions(-) create mode 100644 src/hooks/__tests__/unstable_useSlots.test.tsx diff --git a/src/hooks/__tests__/unstable_useSlots.test.tsx b/src/hooks/__tests__/unstable_useSlots.test.tsx new file mode 100644 index 00000000000..e90bd916a2b --- /dev/null +++ b/src/hooks/__tests__/unstable_useSlots.test.tsx @@ -0,0 +1,115 @@ +import React from 'react' +import {renderHook} from '@testing-library/react' +// eslint-disable-next-line camelcase +import {unstable_useSlots} from '../unstable_useSlots' + +function TestComponentA(props: React.PropsWithChildren) { + return
+} + +function TestComponentB(props: React.PropsWithChildren) { + return
+} + +test('extracts elements based on config object', () => { + const children = [, ,
Hello World
] + const slotsConfig = { + a: TestComponentA, + b: TestComponentB, + } + const {result} = renderHook(() => unstable_useSlots(children, slotsConfig)) + expect(result.current).toMatchInlineSnapshot(` + [ + { + "a": , + "b": , + }, + [ +
+ Hello World +
, + ], + ] + `) +}) + +test('handles empty config object', () => { + const children = [, ,
Hello World
] + const slotsConfig = {} + const {result} = renderHook(() => unstable_useSlots(children, slotsConfig)) + expect(result.current).toMatchInlineSnapshot(` + [ + {}, + [ + , + , +
+ Hello World +
, + ], + ] + `) +}) + +test('handles empty children', () => { + const children: React.ReactNode = [] + const slotsConfig = { + a: TestComponentA, + b: TestComponentB, + } + const {result} = renderHook(() => unstable_useSlots(children, slotsConfig)) + expect(result.current).toMatchInlineSnapshot(` + [ + { + "a": undefined, + "b": undefined, + }, + [], + ] + `) +}) + +test('ignores nested slots', () => { + const children = [ + , +
+ +
, + ] + const slotsConfig = { + a: TestComponentA, + b: TestComponentB, + } + const {result} = renderHook(() => unstable_useSlots(children, slotsConfig)) + expect(result.current).toMatchInlineSnapshot(` + [ + { + "a": , + "b": undefined, + }, + [ +
+ +
, + ], + ] + `) +}) + +test('ignores duplicate slots', () => { + const children = [A1, A2] + const slotsConfig = { + a: TestComponentA, + } + const {result} = renderHook(() => unstable_useSlots(children, slotsConfig)) + expect(result.current).toMatchInlineSnapshot(` + [ + { + "a": + A1 + , + }, + [], + ] + `) +}) diff --git a/src/hooks/unstable_useSlots.ts b/src/hooks/unstable_useSlots.ts index ebd6549d3ea..17ff187c459 100644 --- a/src/hooks/unstable_useSlots.ts +++ b/src/hooks/unstable_useSlots.ts @@ -32,13 +32,19 @@ export function unstable_useSlots( // Check if the child is a slot const slotKey = getKeyByValue(config, child.type) - if (slotKey && !slots[slotKey]) { + if (slotKey && slots[slotKey]) { + // If slot is already filled, ignore duplicates + return + } + + if (slotKey) { // If the child is a slot, add it to the `slots` object slots[slotKey] = child - } else { - // If the child is not a slot, add it to the `rest` array - rest.push(child) + return } + + // If the child is not a slot, add it to the `rest` array + rest.push(child) }) return [slots, rest] From efa40e7174a9d9633a980335a9b15410ef6a7d79 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Wed, 15 Mar 2023 14:11:49 -0600 Subject: [PATCH 3/9] Create .changeset/large-dolls-juggle.md --- .changeset/large-dolls-juggle.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/large-dolls-juggle.md diff --git a/.changeset/large-dolls-juggle.md b/.changeset/large-dolls-juggle.md new file mode 100644 index 00000000000..9d0bef2b2ca --- /dev/null +++ b/.changeset/large-dolls-juggle.md @@ -0,0 +1,7 @@ +--- +"@primer/react": patch +--- + +`PageLayout` and `SplitPageLayout` are now SSR-compatible. + +Warning: In this new implementation, `PageLayout.Header` and `PageLayout.Footer` must be direct children of `PageLayout`. The same applies to `SplitPageLayout` From 5672a0c2f887714a8cbea16272666682f67afe63 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Wed, 15 Mar 2023 15:14:08 -0600 Subject: [PATCH 4/9] =?UTF-8?q?unstable=5FuseSlots=20=E2=86=92=20useSlots?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/PageLayout/PageLayout.tsx | 5 ++--- ...unstable_useSlots.test.tsx => useSlots.test.tsx} | 13 ++++++------- src/hooks/{unstable_useSlots.ts => useSlots.ts} | 3 +-- 3 files changed, 9 insertions(+), 12 deletions(-) rename src/hooks/__tests__/{unstable_useSlots.test.tsx => useSlots.test.tsx} (82%) rename src/hooks/{unstable_useSlots.ts => useSlots.ts} (95%) diff --git a/src/PageLayout/PageLayout.tsx b/src/PageLayout/PageLayout.tsx index 86ac633c64d..b8a0a802a1d 100644 --- a/src/PageLayout/PageLayout.tsx +++ b/src/PageLayout/PageLayout.tsx @@ -1,8 +1,7 @@ import React from 'react' import {createGlobalStyle} from 'styled-components' import Box from '../Box' -// eslint-disable-next-line camelcase -import {unstable_useSlots} from '../hooks/unstable_useSlots' +import {useSlots} from '../hooks/useSlots' import {useId} from '../hooks/useId' import {useRefObjectAsForwardedRef} from '../hooks/useRefObjectAsForwardedRef' import {isResponsiveValue, ResponsiveValue, useResponsiveValue} from '../hooks/useResponsiveValue' @@ -71,7 +70,7 @@ const Root: React.FC> = ({ const {rootRef, enableStickyPane, disableStickyPane, contentTopRef, contentBottomRef, stickyPaneHeight} = useStickyPaneHeight() - const [slots, rest] = unstable_useSlots(children, { + const [slots, rest] = useSlots(children, { header: Header, footer: Footer, }) diff --git a/src/hooks/__tests__/unstable_useSlots.test.tsx b/src/hooks/__tests__/useSlots.test.tsx similarity index 82% rename from src/hooks/__tests__/unstable_useSlots.test.tsx rename to src/hooks/__tests__/useSlots.test.tsx index e90bd916a2b..9b7cbf3790e 100644 --- a/src/hooks/__tests__/unstable_useSlots.test.tsx +++ b/src/hooks/__tests__/useSlots.test.tsx @@ -1,7 +1,6 @@ import React from 'react' import {renderHook} from '@testing-library/react' -// eslint-disable-next-line camelcase -import {unstable_useSlots} from '../unstable_useSlots' +import {useSlots} from '../useSlots' function TestComponentA(props: React.PropsWithChildren) { return
@@ -17,7 +16,7 @@ test('extracts elements based on config object', () => { a: TestComponentA, b: TestComponentB, } - const {result} = renderHook(() => unstable_useSlots(children, slotsConfig)) + const {result} = renderHook(() => useSlots(children, slotsConfig)) expect(result.current).toMatchInlineSnapshot(` [ { @@ -36,7 +35,7 @@ test('extracts elements based on config object', () => { test('handles empty config object', () => { const children = [, ,
Hello World
] const slotsConfig = {} - const {result} = renderHook(() => unstable_useSlots(children, slotsConfig)) + const {result} = renderHook(() => useSlots(children, slotsConfig)) expect(result.current).toMatchInlineSnapshot(` [ {}, @@ -57,7 +56,7 @@ test('handles empty children', () => { a: TestComponentA, b: TestComponentB, } - const {result} = renderHook(() => unstable_useSlots(children, slotsConfig)) + const {result} = renderHook(() => useSlots(children, slotsConfig)) expect(result.current).toMatchInlineSnapshot(` [ { @@ -80,7 +79,7 @@ test('ignores nested slots', () => { a: TestComponentA, b: TestComponentB, } - const {result} = renderHook(() => unstable_useSlots(children, slotsConfig)) + const {result} = renderHook(() => useSlots(children, slotsConfig)) expect(result.current).toMatchInlineSnapshot(` [ { @@ -101,7 +100,7 @@ test('ignores duplicate slots', () => { const slotsConfig = { a: TestComponentA, } - const {result} = renderHook(() => unstable_useSlots(children, slotsConfig)) + const {result} = renderHook(() => useSlots(children, slotsConfig)) expect(result.current).toMatchInlineSnapshot(` [ { diff --git a/src/hooks/unstable_useSlots.ts b/src/hooks/useSlots.ts similarity index 95% rename from src/hooks/unstable_useSlots.ts rename to src/hooks/useSlots.ts index 17ff187c459..8c21ce8118e 100644 --- a/src/hooks/unstable_useSlots.ts +++ b/src/hooks/useSlots.ts @@ -11,8 +11,7 @@ type SlotElements = { * allowing us to implement components with SSR-compatible slot APIs. * Note: We can only extract direct children, not nested ones. */ -// eslint-disable-next-line camelcase -export function unstable_useSlots( +export function useSlots( children: React.ReactNode, config: T, ): [Partial>, React.ReactNode[]] { From 1dd3516c775ce55c55f6a40c794233e55df966e3 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Wed, 15 Mar 2023 15:25:44 -0600 Subject: [PATCH 5/9] Add private _slotsConfig prop to PageLayout --- src/PageLayout/PageLayout.tsx | 11 ++++++----- src/SplitPageLayout/SplitPageLayout.tsx | 14 +++++++++++++- src/hooks/useSlots.ts | 2 +- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/PageLayout/PageLayout.tsx b/src/PageLayout/PageLayout.tsx index b8a0a802a1d..e7efcb82cb4 100644 --- a/src/PageLayout/PageLayout.tsx +++ b/src/PageLayout/PageLayout.tsx @@ -1,10 +1,10 @@ import React from 'react' import {createGlobalStyle} from 'styled-components' import Box from '../Box' -import {useSlots} from '../hooks/useSlots' import {useId} from '../hooks/useId' import {useRefObjectAsForwardedRef} from '../hooks/useRefObjectAsForwardedRef' import {isResponsiveValue, ResponsiveValue, useResponsiveValue} from '../hooks/useResponsiveValue' +import {useSlots} from '../hooks/useSlots' import {BetterSystemStyleObject, merge, SxProp} from '../sx' import {Theme} from '../ThemeProvider' import {canUseDOM} from '../utils/environment' @@ -49,6 +49,9 @@ export type PageLayoutProps = { padding?: keyof typeof SPACING_MAP rowGap?: keyof typeof SPACING_MAP columnGap?: keyof typeof SPACING_MAP + + /** Private prop to allow SplitPageLayout to customize slot components */ + _slotsConfig?: Record<'header' | 'footer', React.ComponentType> } & SxProp const containerWidths = { @@ -66,14 +69,12 @@ const Root: React.FC> = ({ columnGap = 'normal', children, sx = {}, + _slotsConfig: slotsConfig, }) => { const {rootRef, enableStickyPane, disableStickyPane, contentTopRef, contentBottomRef, stickyPaneHeight} = useStickyPaneHeight() - const [slots, rest] = useSlots(children, { - header: Header, - footer: Footer, - }) + const [slots, rest] = useSlots(children, slotsConfig ?? {header: Header, footer: Footer}) return ( > = props => { - return + return ( + + ) } Root.displayName = 'SplitPageLayout' diff --git a/src/hooks/useSlots.ts b/src/hooks/useSlots.ts index 8c21ce8118e..4e158414e78 100644 --- a/src/hooks/useSlots.ts +++ b/src/hooks/useSlots.ts @@ -1,6 +1,6 @@ import React from 'react' -type SlotConfig = Record +export type SlotConfig = Record type SlotElements = { [Property in keyof Type]: React.ReactElement From f588eebec790a3691ac3772b29d606c98a893b57 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Wed, 15 Mar 2023 18:24:53 -0600 Subject: [PATCH 6/9] Improve slots performance --- src/hooks/useSlots.ts | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/hooks/useSlots.ts b/src/hooks/useSlots.ts index 4e158414e78..2b2fb0745c3 100644 --- a/src/hooks/useSlots.ts +++ b/src/hooks/useSlots.ts @@ -21,6 +21,9 @@ export function useSlots( // Array of elements that are not slots const rest: React.ReactNode[] = [] + const keys = Object.keys(config) as Array; + const values = Object.values(config); + // eslint-disable-next-line github/array-foreach React.Children.forEach(children, child => { if (!React.isValidElement(child)) { @@ -28,22 +31,24 @@ export function useSlots( return } - // Check if the child is a slot - const slotKey = getKeyByValue(config, child.type) + const index = values.findIndex(value => { + return child.type === value; + }); - if (slotKey && slots[slotKey]) { - // If slot is already filled, ignore duplicates - return + // If the child is not a slot, add it to the `rest` array + if (index === -1) { + rest.push(child) } - if (slotKey) { - // If the child is a slot, add it to the `slots` object - slots[slotKey] = child - return + const slotKey = keys[index]; + + // If slot is already filled, ignore duplicates + if (slots[slotKey]) { + return; } - // If the child is not a slot, add it to the `rest` array - rest.push(child) + // If the child is a slot, add it to the `slots` object + slots[slotKey] = child }) return [slots, rest] @@ -56,9 +61,3 @@ function mapValues, V>(obj: T, fn: (value: T[k return result }, {} as Record) } - -/** Get the key of an object by its value */ -function getKeyByValue>(object: T, value: unknown): keyof T | undefined { - const keys = Object.keys(object) as (keyof T)[] - return keys.find(key => object[key] === value) -} From 462c59515400dee4ab9d5a02af07c1c52e8f9941 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Wed, 15 Mar 2023 18:37:08 -0600 Subject: [PATCH 7/9] Warn about duplicate slots in development --- src/hooks/__tests__/useSlots.test.tsx | 4 +++- src/hooks/useSlots.ts | 15 +++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/hooks/__tests__/useSlots.test.tsx b/src/hooks/__tests__/useSlots.test.tsx index 9b7cbf3790e..a99572779f7 100644 --- a/src/hooks/__tests__/useSlots.test.tsx +++ b/src/hooks/__tests__/useSlots.test.tsx @@ -95,7 +95,8 @@ test('ignores nested slots', () => { `) }) -test('ignores duplicate slots', () => { +test('warns about duplicate slots', () => { + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}) const children = [A1, A2] const slotsConfig = { a: TestComponentA, @@ -111,4 +112,5 @@ test('ignores duplicate slots', () => { [], ] `) + expect(warnSpy).toHaveBeenCalledTimes(1) }) diff --git a/src/hooks/useSlots.ts b/src/hooks/useSlots.ts index 2b2fb0745c3..6c1de73a3dd 100644 --- a/src/hooks/useSlots.ts +++ b/src/hooks/useSlots.ts @@ -1,4 +1,5 @@ import React from 'react' +import {warning} from '../utils/warning' export type SlotConfig = Record @@ -21,8 +22,8 @@ export function useSlots( // Array of elements that are not slots const rest: React.ReactNode[] = [] - const keys = Object.keys(config) as Array; - const values = Object.values(config); + const keys = Object.keys(config) as Array + const values = Object.values(config) // eslint-disable-next-line github/array-foreach React.Children.forEach(children, child => { @@ -32,19 +33,21 @@ export function useSlots( } const index = values.findIndex(value => { - return child.type === value; - }); + return child.type === value + }) // If the child is not a slot, add it to the `rest` array if (index === -1) { rest.push(child) + return } - const slotKey = keys[index]; + const slotKey = keys[index] // If slot is already filled, ignore duplicates if (slots[slotKey]) { - return; + warning(true, `Found duplicate "${String(slotKey)}" slot. Only the first will be rendered.`) + return } // If the child is a slot, add it to the `slots` object From b8c7aaf9b7aae8b0d0debfc526dfed13a0ea5bec Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 16 Mar 2023 12:18:23 -0600 Subject: [PATCH 8/9] Update snapshots --- .../SplitPageLayout.test.tsx.snap | 82 +++++++++---------- 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/src/SplitPageLayout/__snapshots__/SplitPageLayout.test.tsx.snap b/src/SplitPageLayout/__snapshots__/SplitPageLayout.test.tsx.snap index 19bb4c0bf08..c35d5dc97cd 100644 --- a/src/SplitPageLayout/__snapshots__/SplitPageLayout.test.tsx.snap +++ b/src/SplitPageLayout/__snapshots__/SplitPageLayout.test.tsx.snap @@ -19,29 +19,15 @@ exports[`SplitPageLayout renders default layout 1`] = ` } .c2 { - display: -webkit-box; - display: -webkit-flex; - display: -ms-flexbox; - display: flex; - -webkit-flex: 1 1 100%; - -ms-flex: 1 1 100%; - flex: 1 1 100%; - -webkit-flex-wrap: wrap; - -ms-flex-wrap: wrap; - flex-wrap: wrap; - max-width: 100%; -} - -.c3 { width: 100%; margin-bottom: 0; } -.c4 { +.c3 { padding: 16px; } -.c5 { +.c4 { margin-left: 0; margin-right: 0; display: block; @@ -50,6 +36,20 @@ exports[`SplitPageLayout renders default layout 1`] = ` margin-top: 0; } +.c5 { + display: -webkit-box; + display: -webkit-flex; + display: -ms-flexbox; + display: flex; + -webkit-flex: 1 1 100%; + -ms-flex: 1 1 100%; + flex: 1 1 100%; + -webkit-flex-wrap: wrap; + -ms-flex-wrap: wrap; + flex-wrap: wrap; + max-width: 100%; +} + .c6 { display: -webkit-box; display: -webkit-flex; @@ -143,13 +143,13 @@ exports[`SplitPageLayout renders default layout 1`] = ` } @media screen and (min-width:1012px) { - .c4 { + .c3 { padding: 24px; } } @media screen and (min-width:768px) { - .c5 { + .c4 { margin-left: 0 !important; margin-right: 0 !important; } @@ -219,21 +219,21 @@ exports[`SplitPageLayout renders default layout 1`] = `
-
-
-
- Header -
-
-
+ Header +
+
+ +
@@ -265,19 +265,19 @@ exports[`SplitPageLayout renders default layout 1`] = ` Pane
-
-
-
- Footer -
-
+
+
+
+ Footer +
+
From 5dcc159c3d566fc50489775bda7aebf33050d283 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 16 Mar 2023 12:40:26 -0600 Subject: [PATCH 9/9] Change renderHook import --- src/hooks/__tests__/useSlots.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hooks/__tests__/useSlots.test.tsx b/src/hooks/__tests__/useSlots.test.tsx index a99572779f7..d491f90c475 100644 --- a/src/hooks/__tests__/useSlots.test.tsx +++ b/src/hooks/__tests__/useSlots.test.tsx @@ -1,5 +1,5 @@ +import {renderHook} from '@testing-library/react-hooks' import React from 'react' -import {renderHook} from '@testing-library/react' import {useSlots} from '../useSlots' function TestComponentA(props: React.PropsWithChildren) {