From 3ced78df75f537be53321ddb5540f8688b37d67c Mon Sep 17 00:00:00 2001 From: Si Taggart Date: Thu, 12 Oct 2023 14:07:05 -0700 Subject: [PATCH 1/9] fix(select): enable defaultValue to work correctly by removing rehydration hack --- package.json | 13 +++++++++++-- .../paste-core/components/select/src/Select.tsx | 7 +------ .../select/stories/select.stories.tsx | 17 ++++++++++++++++- 3 files changed, 28 insertions(+), 9 deletions(-) diff --git a/package.json b/package.json index 41ef9e98ee..4efc409c62 100644 --- a/package.json +++ b/package.json @@ -6,7 +6,12 @@ "author": "Twilio Inc.", "license": "MIT", "workspaces": { - "packages": ["apps/**/*", "packages/**/*", "templates/**/*", "!packages/paste-core/core-bundle/**/*"] + "packages": [ + "apps/**/*", + "packages/**/*", + "templates/**/*", + "!packages/paste-core/core-bundle/**/*" + ] }, "types": "./types/index.d.ts", "engines": { @@ -228,5 +233,9 @@ } }, "packageManager": "yarn@3.6.3", - "browserslist": ["last 2 versions", "not dead", "not IE 11"] + "browserslist": [ + "last 2 versions", + "not dead", + "not IE 11" + ] } diff --git a/packages/paste-core/components/select/src/Select.tsx b/packages/paste-core/components/select/src/Select.tsx index 6ecb14962e..da57859225 100644 --- a/packages/paste-core/components/select/src/Select.tsx +++ b/packages/paste-core/components/select/src/Select.tsx @@ -82,11 +82,6 @@ const Select = React.forwardRef( }, ref, ) => { - const [showOptions, setShowOptions] = React.useState(false); - React.useEffect(() => { - setShowOptions(true); - }, []); - return ( ( size={multiple ? size : 0} variant={variant} > - {showOptions && children} + {children} {!multiple && ( diff --git a/packages/paste-core/components/select/stories/select.stories.tsx b/packages/paste-core/components/select/stories/select.stories.tsx index d8d06a6231..57fe29ef98 100644 --- a/packages/paste-core/components/select/stories/select.stories.tsx +++ b/packages/paste-core/components/select/stories/select.stories.tsx @@ -45,9 +45,24 @@ export const DefaultSelect = (): React.ReactNode => { ); }; - DefaultSelect.storyName = "Select"; +export const DefaultValueSelect = (): React.ReactNode => { + const uid = useUID(); + return ( + <> + + + Info that helps a user with this field. + + ); +}; + export const SelectRequired = (): React.ReactNode => { const uid = useUID(); const [value, setValue] = React.useState("Select - Required"); From ff8688d96eef94d838852ea60e1dcf83509c6d61 Mon Sep 17 00:00:00 2001 From: Si Taggart Date: Thu, 12 Oct 2023 14:39:04 -0700 Subject: [PATCH 2/9] chore: changeset --- .changeset/thirty-forks-poke.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/thirty-forks-poke.md diff --git a/.changeset/thirty-forks-poke.md b/.changeset/thirty-forks-poke.md new file mode 100644 index 0000000000..b2c393fc54 --- /dev/null +++ b/.changeset/thirty-forks-poke.md @@ -0,0 +1,6 @@ +--- +"@twilio-paste/select": patch +"@twilio-paste/core": patch +--- + +[Select] fix the behaviour of `defaultValue` by removing the rehydration useEffect trick to stop a SSR mismatch. By only rendering the child options on mount via an effect it prevents `defaultValue` from selecting the default value. We will look for other ways of dealing with rehydration mismatches From fb18df9d39da9aa8c0c8ca9cda43519634fdab0f Mon Sep 17 00:00:00 2001 From: Si Taggart Date: Fri, 13 Oct 2023 10:29:42 -0700 Subject: [PATCH 3/9] fix: change the fix to render the entire select on mount --- .changeset/thirty-forks-poke.md | 2 +- .../components/select/src/Select.tsx | 36 ++++++++++++------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/.changeset/thirty-forks-poke.md b/.changeset/thirty-forks-poke.md index b2c393fc54..85c55c5766 100644 --- a/.changeset/thirty-forks-poke.md +++ b/.changeset/thirty-forks-poke.md @@ -3,4 +3,4 @@ "@twilio-paste/core": patch --- -[Select] fix the behaviour of `defaultValue` by removing the rehydration useEffect trick to stop a SSR mismatch. By only rendering the child options on mount via an effect it prevents `defaultValue` from selecting the default value. We will look for other ways of dealing with rehydration mismatches +[Select] fix the behaviour of `defaultValue` by tweaking the rehydration useEffect trick, to stop a SSR mismatch. By only rendering the child options on mount via an effect it prevents `defaultValue` from selecting the default value. We moved to rendering the entire Select element on mount to prevent SSR mismatch, and rendering a placeholder select to prevent layout shift. diff --git a/packages/paste-core/components/select/src/Select.tsx b/packages/paste-core/components/select/src/Select.tsx index da57859225..6d7ed64524 100644 --- a/packages/paste-core/components/select/src/Select.tsx +++ b/packages/paste-core/components/select/src/Select.tsx @@ -82,6 +82,11 @@ const Select = React.forwardRef( }, ref, ) => { + const [showOptions, setShowOptions] = React.useState(false); + React.useEffect(() => { + setShowOptions(true); + }, []); + return ( ( variant={variant} > - - {children} - + {showOptions ? ( + + {children} + + ) : ( + {} + )} {!multiple && ( Date: Mon, 16 Oct 2023 09:18:40 -0500 Subject: [PATCH 4/9] fix: small adjustment --- .../components/select/src/Select.tsx | 33 +++++++++---------- 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/packages/paste-core/components/select/src/Select.tsx b/packages/paste-core/components/select/src/Select.tsx index 6d7ed64524..73ef9e834d 100644 --- a/packages/paste-core/components/select/src/Select.tsx +++ b/packages/paste-core/components/select/src/Select.tsx @@ -87,6 +87,7 @@ const Select = React.forwardRef( setShowOptions(true); }, []); + // N.B. `key` on SelectElement fixes an issue where defaultValue is not respected return ( ( variant={variant} > - {showOptions ? ( - - {children} - - ) : ( - {} - )} + + {showOptions && children} + {!multiple && ( Date: Mon, 16 Oct 2023 09:19:35 -0500 Subject: [PATCH 5/9] chore: undo format --- package.json | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/package.json b/package.json index 4efc409c62..41ef9e98ee 100644 --- a/package.json +++ b/package.json @@ -6,12 +6,7 @@ "author": "Twilio Inc.", "license": "MIT", "workspaces": { - "packages": [ - "apps/**/*", - "packages/**/*", - "templates/**/*", - "!packages/paste-core/core-bundle/**/*" - ] + "packages": ["apps/**/*", "packages/**/*", "templates/**/*", "!packages/paste-core/core-bundle/**/*"] }, "types": "./types/index.d.ts", "engines": { @@ -233,9 +228,5 @@ } }, "packageManager": "yarn@3.6.3", - "browserslist": [ - "last 2 versions", - "not dead", - "not IE 11" - ] + "browserslist": ["last 2 versions", "not dead", "not IE 11"] } From 0207de1a3bf47447616d7668fb03d12c82398fa5 Mon Sep 17 00:00:00 2001 From: TheSisb Date: Mon, 16 Oct 2023 09:22:45 -0500 Subject: [PATCH 6/9] chore: adjust changelog note --- .changeset/thirty-forks-poke.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.changeset/thirty-forks-poke.md b/.changeset/thirty-forks-poke.md index 85c55c5766..1957224b5d 100644 --- a/.changeset/thirty-forks-poke.md +++ b/.changeset/thirty-forks-poke.md @@ -3,4 +3,9 @@ "@twilio-paste/core": patch --- -[Select] fix the behaviour of `defaultValue` by tweaking the rehydration useEffect trick, to stop a SSR mismatch. By only rendering the child options on mount via an effect it prevents `defaultValue` from selecting the default value. We moved to rendering the entire Select element on mount to prevent SSR mismatch, and rendering a placeholder select to prevent layout shift. +[Select] fix the hydration issue which caused the `defaultValue` prop to not be respected. + +Since the React v18 upgrade, we were only rendering the children options after the component and html select +wrapper had mounted. The select would mount with a `defaultValue` of a child that didn't exist, then the +children would be added, so it wouldn't know what value to select. By adding a `key` prop to the select, +it now knows to re-render on mount with the children, and can properly lookup the correct value now. From f64a876ecaa098cbbc8f22711cf06ce09f537e3e Mon Sep 17 00:00:00 2001 From: TheSisb Date: Mon, 16 Oct 2023 16:13:23 -0500 Subject: [PATCH 7/9] fix: logspam and local test running --- .changeset/cold-brooms-type.md | 6 ++++++ jest.config.js | 1 + .../paste-core/components/meter/src/Meter.tsx | 15 ++++++++++----- 3 files changed, 17 insertions(+), 5 deletions(-) create mode 100644 .changeset/cold-brooms-type.md diff --git a/.changeset/cold-brooms-type.md b/.changeset/cold-brooms-type.md new file mode 100644 index 0000000000..e46ef58abf --- /dev/null +++ b/.changeset/cold-brooms-type.md @@ -0,0 +1,6 @@ +--- +"@twilio-paste/meter": patch +"@twilio-paste/core": patch +--- + +[Meter] Remove console.warn log spam regarding aria-label diff --git a/jest.config.js b/jest.config.js index e07b997a9d..ad3e4aa191 100644 --- a/jest.config.js +++ b/jest.config.js @@ -14,6 +14,7 @@ module.exports = { "/packages/(?:.+?)/.next/", "/templates/(?:.+?)/.next/", "/packages/(?:.+?)/.netlify/", + "/.netlify/", ], cacheDirectory: ".jest-cache", coverageDirectory: ".jest-coverage", diff --git a/packages/paste-core/components/meter/src/Meter.tsx b/packages/paste-core/components/meter/src/Meter.tsx index 26af10cac2..1b550a16b5 100644 --- a/packages/paste-core/components/meter/src/Meter.tsx +++ b/packages/paste-core/components/meter/src/Meter.tsx @@ -21,11 +21,6 @@ export interface MeterProps extends HTMLPasteProps<"meter">, Pick( ({ element = "METER", id, minLabel, maxLabel, ...props }, ref) => { const { value = 0, minValue = 0, maxValue = 100 } = props; - const { meterProps } = useMeter(props); - - // Calculate the width of the bar as a percentage - const percentage = (value - minValue) / (maxValue - minValue); - const fillWidth = `${Math.round(percentage * 100)}%`; /* * Since Meter isn't a form element, we cannot use htmlFor from the regular Label @@ -38,6 +33,16 @@ const Meter = React.forwardRef( labelledBy = `${id}${LABEL_SUFFIX}`; } + const { meterProps } = useMeter({ + ...props, + // Appeases useLabel's internal warning about missing labels because we're doing our own thing + "aria-labelledby": labelledBy, + }); + + // Calculate the width of the bar as a percentage + const percentage = (value - minValue) / (maxValue - minValue); + const fillWidth = `${Math.round(percentage * 100)}%`; + return ( Date: Mon, 16 Oct 2023 16:26:54 -0500 Subject: [PATCH 8/9] chore: different approach for tests --- .../components/select/src/Select.tsx | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/packages/paste-core/components/select/src/Select.tsx b/packages/paste-core/components/select/src/Select.tsx index 73ef9e834d..9f02f8210a 100644 --- a/packages/paste-core/components/select/src/Select.tsx +++ b/packages/paste-core/components/select/src/Select.tsx @@ -98,20 +98,24 @@ const Select = React.forwardRef( variant={variant} > - - {showOptions && children} - + {showOptions ? ( + + {children} + + ) : ( + {} + )} {!multiple && ( Date: Tue, 17 Oct 2023 10:47:26 -0500 Subject: [PATCH 9/9] fix(data-grid): tab management issue on re-renders --- .changeset/silly-mirrors-smash.md | 6 ++++++ .../components/data-grid/__tests__/index.spec.tsx | 9 +++++++++ .../paste-core/components/data-grid/src/DataGridCell.tsx | 9 ++++++--- 3 files changed, 21 insertions(+), 3 deletions(-) create mode 100644 .changeset/silly-mirrors-smash.md diff --git a/.changeset/silly-mirrors-smash.md b/.changeset/silly-mirrors-smash.md new file mode 100644 index 0000000000..a42a06defc --- /dev/null +++ b/.changeset/silly-mirrors-smash.md @@ -0,0 +1,6 @@ +--- +"@twilio-paste/data-grid": patch +"@twilio-paste/core": patch +--- + +[Data Grid] Fix issue where form elements in the data-grid that immediately re-render on mount cause the tabIndex management system to faulter. diff --git a/packages/paste-core/components/data-grid/__tests__/index.spec.tsx b/packages/paste-core/components/data-grid/__tests__/index.spec.tsx index 32fc830296..a42af05c80 100644 --- a/packages/paste-core/components/data-grid/__tests__/index.spec.tsx +++ b/packages/paste-core/components/data-grid/__tests__/index.spec.tsx @@ -12,6 +12,8 @@ import { SortableColumnsDataGrid, } from "../stories/index.stories"; +jest.useFakeTimers(); + const checkTagName = (el: Element, name: string): void => expect(el.tagName).toBe(name.toUpperCase()); describe("Data Grid", () => { @@ -177,6 +179,9 @@ describe("Data Grid", () => { if (firstInputCell == null) { throw new Error("cannot find firstInputCell"); } + act(() => { + jest.advanceTimersByTime(300); + }); // Focus the button before the DataGrid beforeDataGridButton.focus(); @@ -231,6 +236,7 @@ describe("Data Grid", () => { // I added this particular sequence because it was a reproducable bug in my manual tests act(() => { + jest.advanceTimersByTime(300); firstThCell.focus(); }); expect(firstThCell).toHaveFocus(); @@ -243,6 +249,9 @@ describe("Data Grid", () => { userEvent.tab(); userEvent.tab(); userEvent.keyboard("{enter}"); + act(() => { + jest.advanceTimersByTime(300); + }); // Bring the focus back to the DataGrid userEvent.tab({ shift: true }); userEvent.tab({ shift: true }); diff --git a/packages/paste-core/components/data-grid/src/DataGridCell.tsx b/packages/paste-core/components/data-grid/src/DataGridCell.tsx index 051c240a85..b95214596f 100644 --- a/packages/paste-core/components/data-grid/src/DataGridCell.tsx +++ b/packages/paste-core/components/data-grid/src/DataGridCell.tsx @@ -101,9 +101,12 @@ export const DataGridCell: React.FC> * When actionable mode changes, toggle the tabindex of the cell and children */ React.useEffect(() => { - if (cellRef.current) { - updateTabIndexForActionable(cellRef.current, dataGridState.actionable); - } + setTimeout(() => { + if (cellRef.current) { + // This delay solves issues around components that re-render immediately on mount, like the Select componenent + updateTabIndexForActionable(cellRef.current, dataGridState.actionable); + } + }, 10); }, [dataGridState.actionable]); return (