fix: useUIDSeed usage and guidelines

chanzuckerberg · May 12, 2022 · 50b1cda · 50b1cda
1 parent fa3dee4
commit 50b1cda
Show file tree

Hide file tree

Showing 5 changed files with 60 additions and 36 deletions.
diff --git a/docs/CODE_GUIDELINES.md b/docs/CODE_GUIDELINES.md
@@ -481,6 +481,38 @@ EDS adheres to the following API naming conventions:
 
 # Accessibility <a name="accessibility"></a>
 
+## Generating IDs
+
+ID attributes used for accessibility (e.g. associating `<label>` and `<input>` elements) should be unique and stable.
+
+We currently use [react-uid](https://www.npmjs.com/package/react-uid) hooks for ID generation. To ensure stable results, they cannot be invoked within conditionals or callbacks.
+
+- `useUID()` is the most common usage.
+
+```tsx
+const generatedId = useUID();
+```
+
+- `useUIDSeed()` generates a stable seed generator for use in iterators.
+
+```tsx
+const getUID = useUIDSeed();
+// you should either pass an object to getUID:
+items.forEach((item) => {
+  const generatedId = getUID(item);
+});
+
+// or pass a constructed string:
+items.forEach((item, index) => {
+  const generatedId = getUID(`item-${index}-aria-labelledby`);
+});
+
+// interpolating an object into a string will NOT work:
+// items.forEach((item) => {
+//   const generatedId = getUID(`${item}-id`);
+// });
+```
+
 ## Tools
 
 - [eslint-plugin-jsx-a11y](https://github.com/jsx-eslint/eslint-plugin-jsx-a11y) evaluates static code for a11y issues. Currently this plugin is configured with the "recommended" settings, which generate linting errors for most rule violations. See [this chart](https://github.com/jsx-eslint/eslint-plugin-jsx-a11y#rule-strictness-in-different-modes) for descriptions of each rule.

diff --git a/src/components/FileUploadField/FileUploadField.tsx b/src/components/FileUploadField/FileUploadField.tsx
@@ -217,9 +217,10 @@ export const FileUploadField = ({
     }
   }
 
-  function onFileInputChange(e, uidSeed) {
+  function onFileInputChange(e, getUID) {
     const fileObjects = e.target.files;
     if (!fileObjects) return;
+    const fileArray = Array.from(fileObjects);
 
     /*
      * 1. Copy existing files from state to a new variable
@@ -234,14 +235,14 @@ export const FileUploadField = ({
     let isError;
 
     /* 2 */
-    fileObjects.forEach((file) => {
+    fileArray.forEach((file: File) => {
       if (maxFileSize && file.size >= maxFileSize) {
         setFieldNoteState(maxFileSizeErrorText);
         setIsErrorState(true);
       } else {
         files.push({
           fileObject: file,
-          id: uidSeed(file),
+          id: getUID(file),
         });
         setIsErrorState(isError);
       }
@@ -291,8 +292,7 @@ export const FileUploadField = ({
   }
 
   const isDisabled = disabled || (filesState && filesState >= maxFiles);
-  // useUIDSeed() generates a stable seed generator for use in iterators.
-  const uidSeed = useUIDSeed();
+  const getUID = useUIDSeed();
 
   const componentClassName = clsx(
     styles['file-upload-field'],
@@ -337,7 +337,7 @@ export const FileUploadField = ({
             isError={isError}
             multiple={multiple}
             name={name}
-            onChange={(e) => onFileInputChange(e, uidSeed)}
+            onChange={(e) => onFileInputChange(e, getUID)}
             placeholder={placeholder}
             readOnly={readOnly}
             required={required}

diff --git a/src/components/ListDetail/ListDetail.stories.tsx b/src/components/ListDetail/ListDetail.stories.tsx
@@ -13,9 +13,9 @@ export default {
 
 const Template: Story<Props> = (args) => (
   <ListDetail {...args}>
-    <ListDetail.Panel title="ListDetail.Panel 1" variant="number">
+    <ListDetail.Panel title="ListDetailPanel 1" variant="number">
       <TextPassage>
-        <h3>ListDetail.Panel 1</h3>
+        <h3>ListDetailPanel 1</h3>
         <p>
           Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do
           eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad
@@ -25,9 +25,9 @@ const Template: Story<Props> = (args) => (
       </TextPassage>
     </ListDetail.Panel>
 
-    <ListDetail.Panel title="ListDetail.Panel 2" variant="error">
+    <ListDetail.Panel title="ListDetailPanel 2" variant="error">
       <TextPassage>
-        <h3>ListDetail.Panel 2</h3>
+        <h3>ListDetailPanel 2</h3>
         <p>
           Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do
           eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad
@@ -37,9 +37,9 @@ const Template: Story<Props> = (args) => (
       </TextPassage>
     </ListDetail.Panel>
 
-    <ListDetail.Panel title="ListDetail.Panel 3" variant="warning">
+    <ListDetail.Panel title="ListDetailPanel 3" variant="warning">
       <TextPassage>
-        <h3>ListDetail.Panel 3</h3>
+        <h3>ListDetailPanel 3</h3>
         <p>
           Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do
           eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad
@@ -49,9 +49,9 @@ const Template: Story<Props> = (args) => (
       </TextPassage>
     </ListDetail.Panel>
 
-    <ListDetail.Panel title="ListDetail.Panel 4" variant="success">
+    <ListDetail.Panel title="ListDetailPanel 4" variant="success">
       <TextPassage>
-        <h3>ListDetail.Panel 4</h3>
+        <h3>ListDetailPanel 4</h3>
         <p>
           Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do
           eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad
@@ -61,9 +61,9 @@ const Template: Story<Props> = (args) => (
       </TextPassage>
     </ListDetail.Panel>
 
-    <ListDetail.Panel title="ListDetail.Panel 5">
+    <ListDetail.Panel title="ListDetailPanel 5">
       <TextPassage>
-        <h3>ListDetail.Panel 5</h3>
+        <h3>ListDetailPanel 5</h3>
         <p>
           Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do
           eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad

diff --git a/src/components/ListDetail/ListDetail.tsx b/src/components/ListDetail/ListDetail.tsx
@@ -111,10 +111,9 @@ export const ListDetail = ({
   const listDetailItemRefs = listDetailItems().map(() => React.createRef());
 
   // we can't use the hook in an iterator like this, so generate the base and increment if needed
-  const [idVar, setId] = useState([]);
-  const [ariaLabelledByVar, setAriaLabelledBy] = useState([]);
-  // useUIDSeed() generates a stable seed generator for use in iterators.
-  const uidSeed = useUIDSeed();
+  const [idVar, setId] = useState<string[]>([]);
+  const [ariaLabelledByVar, setAriaLabelledBy] = useState<string[]>([]);
+  const getUID = useUIDSeed();
 
   /**
    * Get previous prop
@@ -150,17 +149,15 @@ export const ListDetail = ({
   useEffect(() => {
     setId(
       listDetailItems().map((item) =>
-        item.props.id ? item.props.id : uidSeed(`${item}-id`),
+        item.props.id ? item.props.id : getUID(item),
       ),
     );
     setAriaLabelledBy(
       listDetailItems().map((item) =>
-        item.props.ariaLabelledBy
-          ? item.props.ariaLabelledBy
-          : uidSeed(`${item}-aria-labelledby`),
+        item.props.ariaLabelledBy ? item.props.ariaLabelledBy : getUID(item),
       ),
     );
-  }, [listDetailItems, uidSeed]);
+  }, [listDetailItems, getUID]);
 
   /**
    * On open

diff --git a/src/components/Tabs/Tabs.tsx b/src/components/Tabs/Tabs.tsx
@@ -127,10 +127,9 @@ export const Tabs = ({
   const tabRefs = tabs().map(() => React.createRef());
 
   // we can't use the hook in an iterator like this, so generate the base and increment if needed
-  const [idVar, setId] = useState([]);
-  const [ariaLabelledByVar, setAriaLabelledBy] = useState([]);
-  // useUIDSeed() generates a stable seed generator for use in iterators.
-  const uidSeed = useUIDSeed();
+  const [idVar, setId] = useState<string[]>([]);
+  const [ariaLabelledByVar, setAriaLabelledBy] = useState<string[]>([]);
+  const getUID = useUIDSeed();
 
   /**
    * Get previous prop
@@ -164,17 +163,13 @@ export const Tabs = ({
    * Autogenerate ids on tabs if not defined.
    */
   useEffect(() => {
-    setId(
-      tabs().map((tab) => (tab.props.id ? tab.props.id : uidSeed(`${tab}-id`))),
-    );
+    setId(tabs().map((tab) => (tab.props.id ? tab.props.id : getUID(tab))));
     setAriaLabelledBy(
       tabs().map((tab) =>
-        tab.props.ariaLabelledBy
-          ? tab.props.ariaLabelledBy
-          : uidSeed(`${tab}-aria-labelledby`),
+        tab.props.ariaLabelledBy ? tab.props.ariaLabelledBy : getUID(tab),
       ),
     );
-  }, [tabs, uidSeed]);
+  }, [tabs, getUID]);
 
   /**
    * On open