From 99893a4e4acc64537cd6219af371a45521dc93e3 Mon Sep 17 00:00:00 2001
From: Conrad Chan <conradchan@box.com>
Date: Fri, 14 May 2021 11:54:00 -0700
Subject: [PATCH 1/3] fix(controls): Add forwardRef to Dropdown and Checkbox

---
 src/lib/types/react-augment.d.ts              |   7 ++
 .../settings/SettingsCheckboxItem.tsx         |   8 +-
 .../controls/settings/SettingsDropdown.scss   |   3 +-
 .../controls/settings/SettingsDropdown.tsx    | 110 ++++++++++--------
 .../__tests__/SettingsDropdown-test.tsx       |  23 +++-
 5 files changed, 100 insertions(+), 51 deletions(-)
 create mode 100644 src/lib/types/react-augment.d.ts

diff --git a/src/lib/types/react-augment.d.ts b/src/lib/types/react-augment.d.ts
new file mode 100644
index 000000000..887fdc93e
--- /dev/null
+++ b/src/lib/types/react-augment.d.ts
@@ -0,0 +1,7 @@
+import React from 'react';
+
+declare module 'react' {
+    function forwardRef<T, P = {}>(
+        render: (props: P, ref: React.ForwardedRef<T>) => React.ReactElement | null,
+    ): (props: P & RefAttributes<T>) => React.ReactElement | null;
+}
diff --git a/src/lib/viewers/controls/settings/SettingsCheckboxItem.tsx b/src/lib/viewers/controls/settings/SettingsCheckboxItem.tsx
index 5a4e6588b..9ffda9624 100644
--- a/src/lib/viewers/controls/settings/SettingsCheckboxItem.tsx
+++ b/src/lib/viewers/controls/settings/SettingsCheckboxItem.tsx
@@ -8,7 +8,10 @@ export type Props = {
     onChange: (isChecked: boolean) => void;
 };
 
-export default function SettingsCheckboxItem({ isChecked, label, onChange }: Props): JSX.Element {
+export type SettingsCheckboxItemRef = HTMLInputElement;
+
+function SettingsCheckboxItem(props: Props, ref: React.Ref<SettingsCheckboxItemRef>): JSX.Element {
+    const { isChecked, label, onChange } = props;
     const { current: id } = React.useRef(uniqueId('bp-SettingsCheckboxItem_'));
 
     const handleChange = (event: React.ChangeEvent<HTMLInputElement>): void => {
@@ -18,6 +21,7 @@ export default function SettingsCheckboxItem({ isChecked, label, onChange }: Pro
     return (
         <div className="bp-SettingsCheckboxItem">
             <input
+                ref={ref}
                 checked={isChecked}
                 className="bp-SettingsCheckboxItem-input"
                 id={id}
@@ -30,3 +34,5 @@ export default function SettingsCheckboxItem({ isChecked, label, onChange }: Pro
         </div>
     );
 }
+
+export default React.forwardRef(SettingsCheckboxItem);
diff --git a/src/lib/viewers/controls/settings/SettingsDropdown.scss b/src/lib/viewers/controls/settings/SettingsDropdown.scss
index 4b81c45f3..f4b033b72 100644
--- a/src/lib/viewers/controls/settings/SettingsDropdown.scss
+++ b/src/lib/viewers/controls/settings/SettingsDropdown.scss
@@ -2,7 +2,8 @@
 
 $bp-SettingsDropdown-spacing: 5px;
 
-.bp-SettingsDropdown {
+.bp-SettingsDropdown,
+.bp-SettingsDropdown-container {
     position: relative;
     display: flex;
     flex: 1 1 auto;
diff --git a/src/lib/viewers/controls/settings/SettingsDropdown.tsx b/src/lib/viewers/controls/settings/SettingsDropdown.tsx
index 7593b3726..78455d95c 100644
--- a/src/lib/viewers/controls/settings/SettingsDropdown.tsx
+++ b/src/lib/viewers/controls/settings/SettingsDropdown.tsx
@@ -22,13 +22,10 @@ export type Props<V extends Value = string> = {
     value?: V;
 };
 
-export default function SettingsDropdown<V extends Value = string>({
-    className,
-    label,
-    listItems,
-    onSelect,
-    value,
-}: Props<V>): JSX.Element {
+export type SettingsDropdownRef = Pick<HTMLButtonElement, 'focus'>;
+
+function SettingsDropdown<V extends Value = string>(props: Props<V>, ref: React.Ref<SettingsDropdownRef>): JSX.Element {
+    const { className, label, listItems, onSelect, value } = props;
     const { current: id } = React.useRef(uniqueId('bp-SettingsDropdown_'));
     const buttonElRef = React.useRef<HTMLButtonElement | null>(null);
     const dropdownElRef = React.useRef<HTMLDivElement | null>(null);
@@ -74,52 +71,69 @@ export default function SettingsDropdown<V extends Value = string>({
 
     useClickOutside(dropdownElRef, () => setIsOpen(false));
 
+    // Customize the forwarded ref to combine usage with the ref internal to this component
+    React.useImperativeHandle(
+        ref,
+        () => ({
+            focus: (): void => {
+                if (buttonElRef.current) {
+                    buttonElRef.current.focus();
+                }
+            },
+        }),
+        [],
+    );
+
     return (
-        <div ref={dropdownElRef} className={classNames('bp-SettingsDropdown', className)}>
+        <div className={classNames('bp-SettingsDropdown', className)}>
             <div className="bp-SettingsDropdown-label" id={`${id}-label`}>
                 {label}
             </div>
-            <button
-                ref={buttonElRef}
-                aria-expanded={isOpen}
-                aria-haspopup="listbox"
-                aria-labelledby={`${id}-label ${id}-button`}
-                className={classNames('bp-SettingsDropdown-button', { 'bp-is-open': isOpen })}
-                id={`${id}-button`}
-                onClick={(): void => setIsOpen(!isOpen)}
-                type="button"
-            >
-                {value}
-            </button>
-            <SettingsFlyout className="bp-SettingsDropdown-flyout" isOpen={isOpen}>
-                <SettingsList
-                    ref={listElRef}
-                    aria-labelledby={`${id}-label`}
-                    className="bp-SettingsDropdown-list"
-                    isActive
-                    onKeyDown={handleKeyDown}
-                    role="listbox"
-                    tabIndex={-1}
+            <div ref={dropdownElRef} className="bp-SettingsDropdown-container">
+                <button
+                    ref={buttonElRef}
+                    aria-expanded={isOpen}
+                    aria-haspopup="listbox"
+                    aria-labelledby={`${id}-label ${id}-button`}
+                    className={classNames('bp-SettingsDropdown-button', { 'bp-is-open': isOpen })}
+                    id={`${id}-button`}
+                    onClick={(): void => setIsOpen(!isOpen)}
+                    type="button"
                 >
-                    {listItems.map(({ label: itemLabel, value: itemValue }) => {
-                        const itemValueString = itemValue.toString();
-                        return (
-                            <div
-                                key={itemValueString}
-                                aria-selected={value === itemValue}
-                                className="bp-SettingsDropdown-listitem"
-                                id={itemValueString}
-                                onClick={createClickHandler(itemValue)}
-                                onKeyDown={createKeyDownHandler(itemValue)}
-                                role="option"
-                                tabIndex={0}
-                            >
-                                {itemLabel}
-                            </div>
-                        );
-                    })}
-                </SettingsList>
-            </SettingsFlyout>
+                    {value}
+                </button>
+                <SettingsFlyout className="bp-SettingsDropdown-flyout" isOpen={isOpen}>
+                    <SettingsList
+                        ref={listElRef}
+                        aria-labelledby={`${id}-label`}
+                        className="bp-SettingsDropdown-list"
+                        isActive
+                        onKeyDown={handleKeyDown}
+                        role="listbox"
+                        tabIndex={-1}
+                    >
+                        {listItems.map(({ label: itemLabel, value: itemValue }) => {
+                            const itemValueString = itemValue.toString();
+                            return (
+                                <div
+                                    key={itemValueString}
+                                    aria-selected={value === itemValue}
+                                    className="bp-SettingsDropdown-listitem"
+                                    id={itemValueString}
+                                    onClick={createClickHandler(itemValue)}
+                                    onKeyDown={createKeyDownHandler(itemValue)}
+                                    role="option"
+                                    tabIndex={0}
+                                >
+                                    {itemLabel}
+                                </div>
+                            );
+                        })}
+                    </SettingsList>
+                </SettingsFlyout>
+            </div>
         </div>
     );
 }
+
+export default React.forwardRef(SettingsDropdown);
diff --git a/src/lib/viewers/controls/settings/__tests__/SettingsDropdown-test.tsx b/src/lib/viewers/controls/settings/__tests__/SettingsDropdown-test.tsx
index 31b2cb7f2..46c2bb983 100644
--- a/src/lib/viewers/controls/settings/__tests__/SettingsDropdown-test.tsx
+++ b/src/lib/viewers/controls/settings/__tests__/SettingsDropdown-test.tsx
@@ -1,7 +1,7 @@
 import React from 'react';
 import { act } from 'react-dom/test-utils';
 import { mount, ReactWrapper } from 'enzyme';
-import SettingsDropdown, { Props } from '../SettingsDropdown';
+import SettingsDropdown, { Props, SettingsDropdownRef } from '../SettingsDropdown';
 import SettingsFlyout from '../SettingsFlyout';
 import SettingsList from '../SettingsList';
 
@@ -186,6 +186,27 @@ describe('SettingsDropdown', () => {
         });
     });
 
+    describe('ref', () => {
+        const TestComponent = (): JSX.Element => {
+            const ref = React.useRef<SettingsDropdownRef | null>(null);
+
+            React.useEffect(() => {
+                if (ref.current) {
+                    ref.current.focus();
+                }
+            }, []);
+            return <SettingsDropdown ref={ref} {...getDefaults()} />;
+        };
+
+        test('should be able to focus on the dropdown button', () => {
+            const wrapper = mount(<TestComponent />, {
+                attachTo: getHostNode(),
+            });
+
+            expect(wrapper.find('.bp-SettingsDropdown-button').getDOMNode()).toHaveFocus();
+        });
+    });
+
     describe('render', () => {
         test('should return a valid wrapper', () => {
             const wrapper = getWrapper();

From a325ef12dee91a13eff8d73d1528c99b76f13c16 Mon Sep 17 00:00:00 2001
From: Conrad Chan <conradchan@box.com>
Date: Fri, 14 May 2021 14:31:27 -0700
Subject: [PATCH 2/3] chore: pr comments

---
 .../viewers/controls/settings/SettingsDropdown.scss   | 11 ++++++++---
 .../viewers/controls/settings/SettingsDropdown.tsx    |  4 +---
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/lib/viewers/controls/settings/SettingsDropdown.scss b/src/lib/viewers/controls/settings/SettingsDropdown.scss
index f4b033b72..20ff11812 100644
--- a/src/lib/viewers/controls/settings/SettingsDropdown.scss
+++ b/src/lib/viewers/controls/settings/SettingsDropdown.scss
@@ -2,15 +2,20 @@
 
 $bp-SettingsDropdown-spacing: 5px;
 
-.bp-SettingsDropdown,
-.bp-SettingsDropdown-container {
-    position: relative;
+.bp-SettingsDropdown {
     display: flex;
     flex: 1 1 auto;
     flex-direction: column;
     color: $bdl-gray-62;
 }
 
+.bp-SettingsDropdown-toggle {
+    position: relative;
+    display: flex;
+    flex: 1 1 auto;
+    flex-direction: column;
+}
+
 .bp-SettingsDropdown-flyout {
     top: auto;
     right: 0;
diff --git a/src/lib/viewers/controls/settings/SettingsDropdown.tsx b/src/lib/viewers/controls/settings/SettingsDropdown.tsx
index 78455d95c..9d859a6f4 100644
--- a/src/lib/viewers/controls/settings/SettingsDropdown.tsx
+++ b/src/lib/viewers/controls/settings/SettingsDropdown.tsx
@@ -29,7 +29,6 @@ function SettingsDropdown<V extends Value = string>(props: Props<V>, ref: React.
     const { current: id } = React.useRef(uniqueId('bp-SettingsDropdown_'));
     const buttonElRef = React.useRef<HTMLButtonElement | null>(null);
     const dropdownElRef = React.useRef<HTMLDivElement | null>(null);
-    const listElRef = React.useRef<HTMLDivElement | null>(null);
     const [isOpen, setIsOpen] = React.useState(false);
 
     const handleKeyDown = (event: React.KeyboardEvent): void => {
@@ -89,7 +88,7 @@ function SettingsDropdown<V extends Value = string>(props: Props<V>, ref: React.
             <div className="bp-SettingsDropdown-label" id={`${id}-label`}>
                 {label}
             </div>
-            <div ref={dropdownElRef} className="bp-SettingsDropdown-container">
+            <div ref={dropdownElRef} className="bp-SettingsDropdown-toggle">
                 <button
                     ref={buttonElRef}
                     aria-expanded={isOpen}
@@ -104,7 +103,6 @@ function SettingsDropdown<V extends Value = string>(props: Props<V>, ref: React.
                 </button>
                 <SettingsFlyout className="bp-SettingsDropdown-flyout" isOpen={isOpen}>
                     <SettingsList
-                        ref={listElRef}
                         aria-labelledby={`${id}-label`}
                         className="bp-SettingsDropdown-list"
                         isActive

From e2f044fde7338cfbb492ad3eb64172c0eb9e6562 Mon Sep 17 00:00:00 2001
From: Conrad Chan <conradchan@box.com>
Date: Fri, 14 May 2021 15:07:16 -0700
Subject: [PATCH 3/3] chore: pr comments

---
 .../controls/settings/SettingsCheckboxItem.tsx |  4 ++--
 .../controls/settings/SettingsDropdown.scss    |  2 +-
 .../controls/settings/SettingsDropdown.tsx     | 18 ++++--------------
 .../__tests__/SettingsDropdown-test.tsx        |  2 +-
 4 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/src/lib/viewers/controls/settings/SettingsCheckboxItem.tsx b/src/lib/viewers/controls/settings/SettingsCheckboxItem.tsx
index 9ffda9624..82d062ac1 100644
--- a/src/lib/viewers/controls/settings/SettingsCheckboxItem.tsx
+++ b/src/lib/viewers/controls/settings/SettingsCheckboxItem.tsx
@@ -8,9 +8,9 @@ export type Props = {
     onChange: (isChecked: boolean) => void;
 };
 
-export type SettingsCheckboxItemRef = HTMLInputElement;
+export type Ref = HTMLInputElement;
 
-function SettingsCheckboxItem(props: Props, ref: React.Ref<SettingsCheckboxItemRef>): JSX.Element {
+function SettingsCheckboxItem(props: Props, ref: React.Ref<Ref>): JSX.Element {
     const { isChecked, label, onChange } = props;
     const { current: id } = React.useRef(uniqueId('bp-SettingsCheckboxItem_'));
 
diff --git a/src/lib/viewers/controls/settings/SettingsDropdown.scss b/src/lib/viewers/controls/settings/SettingsDropdown.scss
index 20ff11812..a0a1e8519 100644
--- a/src/lib/viewers/controls/settings/SettingsDropdown.scss
+++ b/src/lib/viewers/controls/settings/SettingsDropdown.scss
@@ -9,7 +9,7 @@ $bp-SettingsDropdown-spacing: 5px;
     color: $bdl-gray-62;
 }
 
-.bp-SettingsDropdown-toggle {
+.bp-SettingsDropdown-content {
     position: relative;
     display: flex;
     flex: 1 1 auto;
diff --git a/src/lib/viewers/controls/settings/SettingsDropdown.tsx b/src/lib/viewers/controls/settings/SettingsDropdown.tsx
index 9d859a6f4..e487db180 100644
--- a/src/lib/viewers/controls/settings/SettingsDropdown.tsx
+++ b/src/lib/viewers/controls/settings/SettingsDropdown.tsx
@@ -22,9 +22,9 @@ export type Props<V extends Value = string> = {
     value?: V;
 };
 
-export type SettingsDropdownRef = Pick<HTMLButtonElement, 'focus'>;
+export type Ref = HTMLButtonElement | null;
 
-function SettingsDropdown<V extends Value = string>(props: Props<V>, ref: React.Ref<SettingsDropdownRef>): JSX.Element {
+function SettingsDropdown<V extends Value = string>(props: Props<V>, ref: React.Ref<Ref>): JSX.Element {
     const { className, label, listItems, onSelect, value } = props;
     const { current: id } = React.useRef(uniqueId('bp-SettingsDropdown_'));
     const buttonElRef = React.useRef<HTMLButtonElement | null>(null);
@@ -71,24 +71,14 @@ function SettingsDropdown<V extends Value = string>(props: Props<V>, ref: React.
     useClickOutside(dropdownElRef, () => setIsOpen(false));
 
     // Customize the forwarded ref to combine usage with the ref internal to this component
-    React.useImperativeHandle(
-        ref,
-        () => ({
-            focus: (): void => {
-                if (buttonElRef.current) {
-                    buttonElRef.current.focus();
-                }
-            },
-        }),
-        [],
-    );
+    React.useImperativeHandle(ref, () => buttonElRef.current, []);
 
     return (
         <div className={classNames('bp-SettingsDropdown', className)}>
             <div className="bp-SettingsDropdown-label" id={`${id}-label`}>
                 {label}
             </div>
-            <div ref={dropdownElRef} className="bp-SettingsDropdown-toggle">
+            <div ref={dropdownElRef} className="bp-SettingsDropdown-content">
                 <button
                     ref={buttonElRef}
                     aria-expanded={isOpen}
diff --git a/src/lib/viewers/controls/settings/__tests__/SettingsDropdown-test.tsx b/src/lib/viewers/controls/settings/__tests__/SettingsDropdown-test.tsx
index 46c2bb983..97bed2284 100644
--- a/src/lib/viewers/controls/settings/__tests__/SettingsDropdown-test.tsx
+++ b/src/lib/viewers/controls/settings/__tests__/SettingsDropdown-test.tsx
@@ -1,7 +1,7 @@
 import React from 'react';
 import { act } from 'react-dom/test-utils';
 import { mount, ReactWrapper } from 'enzyme';
-import SettingsDropdown, { Props, SettingsDropdownRef } from '../SettingsDropdown';
+import SettingsDropdown, { Props, Ref as SettingsDropdownRef } from '../SettingsDropdown';
 import SettingsFlyout from '../SettingsFlyout';
 import SettingsList from '../SettingsList';