From 14fd04a653ac667447089f34144c066fffbc1532 Mon Sep 17 00:00:00 2001 From: Ben Scott Date: Wed, 4 Sep 2019 22:01:42 -0700 Subject: [PATCH] Add useUniqueId hook This hook returns a stable value across multiple rerenders, and can optionally be overridden with a fixed value Use this hook in several functional components that currently have unstable ids that change between rerenders. Populate the idFactory of this hook in the AppProvider, with eye to eventually allowing consumers to override the idFactory so it can be reset between server renders (making this public and configurable is a follow up step) --- UNRELEASED.md | 1 + src/components/AppProvider/AppProvider.tsx | 24 +++- src/components/ChoiceList/ChoiceList.tsx | 7 +- .../FormLayout/components/Group/Group.tsx | 6 +- .../Item/components/Secondary/Secondary.tsx | 10 +- src/components/OptionList/OptionList.tsx | 18 +-- .../components/Checkbox/Checkbox.tsx | 8 +- .../PolarisTestProvider.tsx | 27 ++-- src/components/RadioButton/RadioButton.tsx | 11 +- .../components/ScrollTo/ScrollTo.tsx | 6 +- src/components/Select/Select.tsx | 8 +- src/components/Toast/Toast.tsx | 14 +- src/utilities/unique-id/context.ts | 6 + src/utilities/unique-id/hooks.ts | 27 ++++ src/utilities/unique-id/index.ts | 5 + src/utilities/unique-id/tests/hooks.test.tsx | 133 ++++++++++++++++++ .../unique-id/tests/unique-id-factory.test.ts | 36 +++++ src/utilities/unique-id/unique-id-factory.ts | 25 ++++ 18 files changed, 303 insertions(+), 69 deletions(-) create mode 100644 src/utilities/unique-id/context.ts create mode 100644 src/utilities/unique-id/hooks.ts create mode 100644 src/utilities/unique-id/index.ts create mode 100644 src/utilities/unique-id/tests/hooks.test.tsx create mode 100644 src/utilities/unique-id/tests/unique-id-factory.test.ts create mode 100644 src/utilities/unique-id/unique-id-factory.ts diff --git a/UNRELEASED.md b/UNRELEASED.md index e22a6ef38fa..a5142852b06 100644 --- a/UNRELEASED.md +++ b/UNRELEASED.md @@ -25,5 +25,6 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f ### Code quality - Migrated `ActionMenu.RollupAction`, `Autocomplete`, `Card`, `EmptySearchResult`, `Form`, `SkeletonPage` and `TopBar` to use hooks instead of withAppProvider ([#2065](https://github.com/Shopify/polaris-react/pull/2065)) +- Added `useUniqueId` hook that can be used to get a unique id that remains consistent between rerenders and update components to use it where appropriate ([#2079](https://github.com/Shopify/polaris-react/pull/2079)) ### Deprecations diff --git a/src/components/AppProvider/AppProvider.tsx b/src/components/AppProvider/AppProvider.tsx index f8d3f963f0a..2d385692782 100644 --- a/src/components/AppProvider/AppProvider.tsx +++ b/src/components/AppProvider/AppProvider.tsx @@ -16,6 +16,11 @@ import { StickyManagerContext, } from '../../utilities/sticky-manager'; import {LinkContext, LinkLikeComponent} from '../../utilities/link'; +import { + UniqueIdFactory, + UniqueIdFactoryContext, + globalIdGeneratorFactory, +} from '../../utilities/unique-id'; interface State { intl: I18n; @@ -35,11 +40,14 @@ export interface AppProviderProps extends AppBridgeOptions { export class AppProvider extends React.Component { private stickyManager: StickyManager; private scrollLockManager: ScrollLockManager; + private uniqueIdFactory: UniqueIdFactory; constructor(props: AppProviderProps) { super(props); this.stickyManager = new StickyManager(); this.scrollLockManager = new ScrollLockManager(); + this.uniqueIdFactory = new UniqueIdFactory(globalIdGeneratorFactory); + const {i18n, apiKey, shopOrigin, forceRedirect, linkComponent} = this.props; // eslint-disable-next-line react/state-in-constructor @@ -91,13 +99,15 @@ export class AppProvider extends React.Component { - - - - {React.Children.only(children)} - - - + + + + + {React.Children.only(children)} + + + + diff --git a/src/components/ChoiceList/ChoiceList.tsx b/src/components/ChoiceList/ChoiceList.tsx index 1e02afce763..567a218c39d 100644 --- a/src/components/ChoiceList/ChoiceList.tsx +++ b/src/components/ChoiceList/ChoiceList.tsx @@ -1,7 +1,7 @@ import React from 'react'; -import {createUniqueIDFactory} from '@shopify/javascript-utilities/other'; import {classNames} from '../../utilities/css'; +import {useUniqueId} from '../../utilities/unique-id'; import {Checkbox} from '../Checkbox'; import {RadioButton} from '../RadioButton'; import {InlineError, errorTextID} from '../InlineError'; @@ -48,8 +48,6 @@ export interface ChoiceListProps { onChange?(selected: string[], name: string): void; } -const getUniqueID = createUniqueIDFactory('ChoiceList'); - export function ChoiceList({ title, titleHidden, @@ -59,12 +57,13 @@ export function ChoiceList({ onChange = noop, error, disabled = false, - name = getUniqueID(), + name: nameProp, }: ChoiceListProps) { // Type asserting to any is required for TS3.2 but can be removed when we update to 3.3 // see https://github.com/Microsoft/TypeScript/issues/28768 const ControlComponent: any = allowMultiple ? Checkbox : RadioButton; + const name = useUniqueId('ChoiceList', nameProp); const finalName = allowMultiple ? `${name}[]` : name; const className = classNames( diff --git a/src/components/FormLayout/components/Group/Group.tsx b/src/components/FormLayout/components/Group/Group.tsx index 232fc2a043c..ccc91650cb1 100644 --- a/src/components/FormLayout/components/Group/Group.tsx +++ b/src/components/FormLayout/components/Group/Group.tsx @@ -1,8 +1,8 @@ import React from 'react'; -import {createUniqueIDFactory} from '@shopify/javascript-utilities/other'; import {classNames} from '../../../../utilities/css'; import {wrapWithComponent} from '../../../../utilities/components'; +import {useUniqueId} from '../../../../utilities/unique-id'; import styles from '../../FormLayout.scss'; import {Item} from '../Item'; @@ -13,12 +13,10 @@ export interface GroupProps { helpText?: React.ReactNode; } -const getUniqueID = createUniqueIDFactory('FormLayoutGroup'); - export function Group({children, condensed, title, helpText}: GroupProps) { const className = classNames(condensed ? styles.condensed : styles.grouped); - const id = getUniqueID(); + const id = useUniqueId('FormLayoutGroup'); let helpTextElement = null; let helpTextID: undefined | string; diff --git a/src/components/Navigation/components/Item/components/Secondary/Secondary.tsx b/src/components/Navigation/components/Item/components/Secondary/Secondary.tsx index f175b851db7..a521ec0594f 100644 --- a/src/components/Navigation/components/Item/components/Secondary/Secondary.tsx +++ b/src/components/Navigation/components/Item/components/Secondary/Secondary.tsx @@ -1,23 +1,19 @@ import React from 'react'; -import {createUniqueIDFactory} from '@shopify/javascript-utilities/other'; +import {useUniqueId} from '../../../../../../utilities/unique-id'; import {Collapsible} from '../../../../../Collapsible'; import styles from '../../../../Navigation.scss'; -const createSecondaryNavigationId = createUniqueIDFactory( - 'SecondaryNavigation', -); - interface SecondaryProps { expanded: boolean; children?: React.ReactNode; } export function Secondary({children, expanded}: SecondaryProps) { - const secondaryNavigationId = createSecondaryNavigationId(); + const id = useUniqueId('SecondaryNavigation'); return ( - +
    {children}
); diff --git a/src/components/OptionList/OptionList.tsx b/src/components/OptionList/OptionList.tsx index d10a95b9348..604981788ee 100644 --- a/src/components/OptionList/OptionList.tsx +++ b/src/components/OptionList/OptionList.tsx @@ -1,10 +1,10 @@ -import React, {useState, useRef, useCallback} from 'react'; -import {createUniqueIDFactory} from '@shopify/javascript-utilities/other'; +import React, {useState, useCallback} from 'react'; import {arraysAreEqual} from '../../utilities/arrays'; import {IconProps} from '../../types'; import {AvatarProps} from '../Avatar'; import {ThumbnailProps} from '../Thumbnail'; +import {useUniqueId} from '../../utilities/unique-id'; import {useDeepEffect} from '../../utilities/use-deep-effect'; import {Option} from './components'; @@ -34,8 +34,6 @@ export interface SectionDescriptor { type Descriptor = OptionDescriptor | SectionDescriptor; -const getUniqueId = createUniqueIDFactory('OptionList'); - export interface OptionListProps { /** A unique identifier for the option list */ id?: string; @@ -66,16 +64,12 @@ export function OptionList({ role, optionRole, onChange, - id: propId, + id: idProp, }: OptionListProps) { const [normalizedOptions, setNormalizedOptions] = useState( createNormalizedOptions(options, sections, title), ); - const id = useRef(propId || getUniqueId()); - - if (id.current !== propId) { - id.current = propId || id.current; - } + const id = useUniqueId('OptionList', idProp); useDeepEffect( () => { @@ -122,7 +116,7 @@ export function OptionList({ options.map((option, optionIndex) => { const isSelected = selected.includes(option.value); const optionId = - option.id || `${id.current}-${sectionIndex}-${optionIndex}`; + option.id || `${id}-${sectionIndex}-${optionIndex}`; return (