fix(select): enable defaultValue to work correctly by removing rehydr…

…ation hack (#3545) * fix(select): enable defaultValue to work correctly by removing rehydration hack * chore: changeset * fix: change the fix to render the entire select on mount * fix: small adjustment * chore: undo format * chore: adjust changelog note * fix: logspam and local test running * chore: different approach for tests * fix(data-grid): tab management issue on re-renders --------- Co-authored-by: TheSisb <[email protected]>
twilio-labs · Oct 17, 2023 · bc51057 · bc51057
1 parent f370db0
commit bc51057
Show file tree

Hide file tree

Showing 9 changed files with 84 additions and 22 deletions.
diff --git a/.changeset/cold-brooms-type.md 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/.changeset/silly-mirrors-smash.md 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/.changeset/thirty-forks-poke.md b/.changeset/thirty-forks-poke.md
@@ -0,0 +1,11 @@
+---
+"@twilio-paste/select": patch
+"@twilio-paste/core": patch
+---
+
+[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.
diff --git a/jest.config.js b/jest.config.js
@@ -14,6 +14,7 @@ module.exports = {
     "<rootDir>/packages/(?:.+?)/.next/",
     "<rootDir>/templates/(?:.+?)/.next/",
     "<rootDir>/packages/(?:.+?)/.netlify/",
+    "<rootDir>/.netlify/",
   ],
   cacheDirectory: ".jest-cache",
   coverageDirectory: ".jest-coverage",

diff --git 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
@@ -101,9 +101,12 @@ export const DataGridCell: React.FC<React.PropsWithChildren<DataGridCellProps>>
    * 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 (

diff --git 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<BoxProps, "ele
 const Meter = React.forwardRef<HTMLMeterElement, MeterProps>(
   ({ 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<HTMLMeterElement, MeterProps>(
       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 (
       <Box
         as="div"

diff --git 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<HTMLSelectElement, SelectProps>(
       setShowOptions(true);
     }, []);
 
+    // N.B. `key` on SelectElement fixes an issue where defaultValue is not respected
     return (
       <InputBox
         disabled={disabled}
@@ -97,19 +98,24 @@ const Select = React.forwardRef<HTMLSelectElement, SelectProps>(
         variant={variant}
       >
         <Box display="flex" width="100%" position="relative">
-          <SelectElement
-            aria-invalid={hasError}
-            data-not-selectize="true"
-            disabled={disabled}
-            ref={ref}
-            element={`${element}_ELEMENT`}
-            {...props}
-            multiple={multiple}
-            size={multiple ? size : 0}
-            variant={variant}
-          >
-            {showOptions && children}
-          </SelectElement>
+          {showOptions ? (
+            <SelectElement
+              aria-invalid={hasError}
+              data-not-selectize="true"
+              disabled={disabled}
+              ref={ref}
+              element={`${element}_ELEMENT`}
+              {...props}
+              multiple={multiple}
+              size={multiple ? size : 0}
+              variant={variant}
+              key="mounted"
+            >
+              {children}
+            </SelectElement>
+          ) : (
+            <SelectElement key="unmounted">{}</SelectElement>
+          )}
           {!multiple && (
             <InputChevronWrapper element={`${element}_CHEVRON_WRAPPER`}>
               <ChevronDownIcon

diff --git 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 (
+    <>
+      <Label htmlFor={uid}>Label</Label>
+      <Select id={uid} defaultValue="option-2" onFocus={action("handleFocus")} onBlur={action("handleBlur")}>
+        <Option value="option-1">Option 1</Option>
+        <Option value="option-2">Option 2</Option>
+        <Option value="option-3">Option 3</Option>
+        <Option value="option-4">Option 4</Option>
+      </Select>
+      <HelpText>Info that helps a user with this field.</HelpText>
+    </>
+  );
+};
+
 export const SelectRequired = (): React.ReactNode => {
   const uid = useUID();
   const [value, setValue] = React.useState("Select - Required");