From 76695828721e1953dbd4ea8e050f937a4b336070 Mon Sep 17 00:00:00 2001 From: Ben Scott Date: Thu, 5 Sep 2019 14:37:20 -0700 Subject: [PATCH] More PR Feedback - Use PascalCase for default global prefix - Fix typos - Rejig tests to create elements a little more like what we would write in real components --- src/utilities/unique-id/hooks.ts | 8 +- src/utilities/unique-id/tests/hooks.test.tsx | 110 ++++++++++-------- .../unique-id/tests/unique-id-factory.test.ts | 26 ++--- src/utilities/unique-id/unique-id-factory.ts | 2 +- 4 files changed, 80 insertions(+), 66 deletions(-) diff --git a/src/utilities/unique-id/hooks.ts b/src/utilities/unique-id/hooks.ts index 9a5b46ccdf6..5a0793cbb93 100644 --- a/src/utilities/unique-id/hooks.ts +++ b/src/utilities/unique-id/hooks.ts @@ -2,19 +2,19 @@ import {useContext, useRef} from 'react'; import {UniqueIdFactoryContext} from './context'; /** - * Returns a unique id that remains consistent across multiple rerenders of the + * Returns a unique id that remains consistent across multiple re-renders of the * same hook * @param prefix Defines a prefix for the ID. You probably want to set this to * the name of the component you're calling `useUniqueId` in. * @param overrideId Defines a fixed value to use instead of generating a unique - * ID. Useful for components that allow consumers to specify a fixed ID. + * ID. Useful for components that allow consumers to specify their own ID. */ export function useUniqueId(prefix = '', overrideId = '') { const idFactory = useContext(UniqueIdFactoryContext); - // By using a ref to store the uniqueId for each incovation of the hook and + // By using a ref to store the uniqueId for each invocation of the hook and // checking that it is not already populated we ensure that we don't generate - // a new ID on ever rerender of a component. + // a new ID on every re-render of a component. const uniqueIdRef = useRef(null); if (!idFactory) { diff --git a/src/utilities/unique-id/tests/hooks.test.tsx b/src/utilities/unique-id/tests/hooks.test.tsx index 4ef8076f5c1..c058b834c82 100644 --- a/src/utilities/unique-id/tests/hooks.test.tsx +++ b/src/utilities/unique-id/tests/hooks.test.tsx @@ -6,14 +6,14 @@ function TestHarness({children}: {children: React.ReactNode}) { return {children}; } -const Component1 = () => {useUniqueId()}; -const Component2 = () => {useUniqueId()}; -const Component3 = () => {useUniqueId()}; +const Component1 = () =>
; +const Component2 = () =>
; +const Component3 = () =>
; -const HasPrefix1 = () => {useUniqueId('a')}; -const HasPrefix2 = () => {useUniqueId('a')}; -const HasPrefix3 = () => {useUniqueId('a')}; -const HasPrefix4 = () => {useUniqueId('b')}; +const HasPrefix1 = () =>
; +const HasPrefix2 = () =>
; +const HasPrefix3 = () =>
; +const HasPrefix4 = () =>
; describe('useUniqueId', () => { it('returns unique IDs across a single component', () => { @@ -28,13 +28,12 @@ describe('useUniqueId', () => { , ); - expect(harness.findAll(Component1)[0].html()).toStrictEqual('polaris-1'); - expect(harness.findAll(Component1)[1].html()).toStrictEqual('polaris-2'); - expect(harness.findAll(Component1)[2].html()).toStrictEqual('polaris-3'); - - expect(harness.findAll(HasPrefix1)[0].html()).toStrictEqual('polaris-a1'); - expect(harness.findAll(HasPrefix1)[1].html()).toStrictEqual('polaris-a2'); - expect(harness.findAll(HasPrefix1)[2].html()).toStrictEqual('polaris-a3'); + expect(harness.findAll('div')[0]).toHaveReactProps({id: 'Polaris1'}); + expect(harness.findAll('div')[1]).toHaveReactProps({id: 'Polaris2'}); + expect(harness.findAll('div')[2]).toHaveReactProps({id: 'Polaris3'}); + expect(harness.findAll('div')[3]).toHaveReactProps({id: 'PolarisA1'}); + expect(harness.findAll('div')[4]).toHaveReactProps({id: 'PolarisA2'}); + expect(harness.findAll('div')[5]).toHaveReactProps({id: 'PolarisA3'}); }); it('returns unique IDs across multiple components', () => { @@ -50,41 +49,39 @@ describe('useUniqueId', () => { , ); - expect(harness.find(Component1)!.html()).toStrictEqual('polaris-1'); - expect(harness.find(Component2)!.html()).toStrictEqual('polaris-2'); - expect(harness.find(Component3)!.html()).toStrictEqual('polaris-3'); - - expect(harness.find(HasPrefix1)!.html()).toStrictEqual('polaris-a1'); - expect(harness.find(HasPrefix2)!.html()).toStrictEqual('polaris-a2'); - expect(harness.find(HasPrefix3)!.html()).toStrictEqual('polaris-a3'); - expect(harness.find(HasPrefix4)!.html()).toStrictEqual('polaris-b1'); + expect(harness.findAll('div')[0]).toHaveReactProps({id: 'Polaris1'}); + expect(harness.findAll('div')[1]).toHaveReactProps({id: 'Polaris2'}); + expect(harness.findAll('div')[2]).toHaveReactProps({id: 'Polaris3'}); + expect(harness.findAll('div')[3]).toHaveReactProps({id: 'PolarisA1'}); + expect(harness.findAll('div')[4]).toHaveReactProps({id: 'PolarisA2'}); + expect(harness.findAll('div')[5]).toHaveReactProps({id: 'PolarisA3'}); + expect(harness.findAll('div')[6]).toHaveReactProps({id: 'PolarisB1'}); }); it('increments multiple IDs within the same component', () => { const HasMultipleIds = () => ( - - {useUniqueId()} :: {useUniqueId()} - +
); const harness = mountWithApp(); - expect(harness.html()).toStrictEqual('polaris-1 :: polaris-2'); + expect(harness.find('div')).toHaveReactProps({ + id: 'Polaris1', + title: 'Polaris2', + }); }); it('uses an override if specified', () => { - const HasOverride = () => ( - {useUniqueId('', 'overridden')} - ); + const HasOverride = () =>
; const harness = mountWithApp(); - expect(harness.html()).toStrictEqual('overridden'); + expect(harness.find('div')).toHaveReactProps({id: 'overridden'}); }); it('uses an override if specified and the override does not interupt the count', () => { const HasOverride = ({idOverride}: {idOverride?: string}) => ( - {useUniqueId('', idOverride)} +
); const harness = mountWithApp( @@ -95,16 +92,14 @@ describe('useUniqueId', () => { , ); - expect(harness.findAll(HasOverride)[0].html()).toStrictEqual('polaris-1'); - expect(harness.findAll(HasOverride)[1].html()).toStrictEqual('overridden'); - expect(harness.findAll(HasOverride)[2].html()).toStrictEqual('polaris-2'); + expect(harness.findAll('div')[0]).toHaveReactProps({id: 'Polaris1'}); + expect(harness.findAll('div')[1]).toHaveReactProps({id: 'overridden'}); + expect(harness.findAll('div')[2]).toHaveReactProps({id: 'Polaris2'}); }); it('keeps the same ID across multiple rerenders', () => { const HasProp = ({info}: {info: string}) => ( - - {info} :: {useUniqueId()} - +
); const ReRenderingTestHarness = () => { @@ -124,19 +119,23 @@ describe('useUniqueId', () => { const harness = mountWithApp(); - expect(harness.find(HasProp)!.html()).toStrictEqual('count1 :: polaris-1'); + expect(harness.findAll('div')[0]).toHaveReactProps({ + title: 'count1', + id: 'Polaris1', + }); harness.find('button')!.trigger('onClick'); - expect(harness.find(HasProp)!.html()).toStrictEqual('count2 :: polaris-1'); + expect(harness.findAll('div')[0]).toHaveReactProps({ + title: 'count2', + id: 'Polaris1', + }); }); it('updates the ID if the overridden ID changes', () => { type HasPropProps = {info: string; idOverride?: string}; const HasProp = ({info, idOverride}: HasPropProps) => ( - - {info} :: {useUniqueId('', idOverride)} - +
); const ReRenderingTestHarness = () => { @@ -147,7 +146,7 @@ describe('useUniqueId', () => { ); // eslint-disable-next-line shopify/jest/no-if - const override = count % 2 === 0 ? `override-${count}` : undefined; + const override = count % 2 === 0 ? `Override${count}` : undefined; return ( @@ -160,23 +159,38 @@ describe('useUniqueId', () => { const harness = mountWithApp(); // Initially we use an incremental id - expect(harness.find(HasProp)!.html()).toStrictEqual('count1 :: polaris-1'); + expect(harness.find('div')).toHaveReactProps({ + title: 'count1', + id: 'Polaris1', + }); // But then we set an override id, so it should use that harness.find('button')!.trigger('onClick'); - expect(harness.find(HasProp)!.html()).toStrictEqual('count2 :: override-2'); + expect(harness.find('div')).toHaveReactProps({ + title: 'count2', + id: 'Override2', + }); // Then on the next render we don't set an override id, so we should go back // to using the incremental id harness.find('button')!.trigger('onClick'); - expect(harness.find(HasProp)!.html()).toStrictEqual('count3 :: polaris-1'); + expect(harness.find('div')).toHaveReactProps({ + title: 'count3', + id: 'Polaris1', + }); // Back to setting an override id harness.find('button')!.trigger('onClick'); - expect(harness.find(HasProp)!.html()).toStrictEqual('count4 :: override-4'); + expect(harness.find('div')).toHaveReactProps({ + title: 'count4', + id: 'Override4', + }); // Back to not setting an override, so back to using the incremental id harness.find('button')!.trigger('onClick'); - expect(harness.find(HasProp)!.html()).toStrictEqual('count5 :: polaris-1'); + expect(harness.find('div')).toHaveReactProps({ + title: 'count5', + id: 'Polaris1', + }); }); }); diff --git a/src/utilities/unique-id/tests/unique-id-factory.test.ts b/src/utilities/unique-id/tests/unique-id-factory.test.ts index f53f13dc63c..7658a7737c2 100644 --- a/src/utilities/unique-id/tests/unique-id-factory.test.ts +++ b/src/utilities/unique-id/tests/unique-id-factory.test.ts @@ -4,33 +4,33 @@ describe('UniqueIdFactory', () => { it('returns unique IDs across multiple calls', () => { const uniqueIdFactory = new UniqueIdFactory(globalIdGeneratorFactory); - expect(uniqueIdFactory.nextId('')).toStrictEqual('polaris-1'); - expect(uniqueIdFactory.nextId('')).toStrictEqual('polaris-2'); - expect(uniqueIdFactory.nextId('')).toStrictEqual('polaris-3'); + expect(uniqueIdFactory.nextId('')).toStrictEqual('Polaris1'); + expect(uniqueIdFactory.nextId('')).toStrictEqual('Polaris2'); + expect(uniqueIdFactory.nextId('')).toStrictEqual('Polaris3'); - expect(uniqueIdFactory.nextId('a')).toStrictEqual('polaris-a1'); - expect(uniqueIdFactory.nextId('a')).toStrictEqual('polaris-a2'); - expect(uniqueIdFactory.nextId('a')).toStrictEqual('polaris-a3'); + expect(uniqueIdFactory.nextId('A')).toStrictEqual('PolarisA1'); + expect(uniqueIdFactory.nextId('A')).toStrictEqual('PolarisA2'); + expect(uniqueIdFactory.nextId('A')).toStrictEqual('PolarisA3'); }); it('returns unique IDs across prefixes', () => { const uniqueIdFactory = new UniqueIdFactory(globalIdGeneratorFactory); - expect(uniqueIdFactory.nextId('a')).toStrictEqual('polaris-a1'); - expect(uniqueIdFactory.nextId('a')).toStrictEqual('polaris-a2'); - expect(uniqueIdFactory.nextId('a')).toStrictEqual('polaris-a3'); + expect(uniqueIdFactory.nextId('A')).toStrictEqual('PolarisA1'); + expect(uniqueIdFactory.nextId('A')).toStrictEqual('PolarisA2'); + expect(uniqueIdFactory.nextId('A')).toStrictEqual('PolarisA3'); }); it('can accept a custom factory', () => { const customIdGeneratorFactory = (prefix: string) => { let index = 101; - return () => `custom-${prefix}${index++}`; + return () => `Custom${prefix}${index++}`; }; const uniqueIdFactory = new UniqueIdFactory(customIdGeneratorFactory); - expect(uniqueIdFactory.nextId('')).toStrictEqual('custom-101'); - expect(uniqueIdFactory.nextId('')).toStrictEqual('custom-102'); - expect(uniqueIdFactory.nextId('a')).toStrictEqual('custom-a101'); + expect(uniqueIdFactory.nextId('')).toStrictEqual('Custom101'); + expect(uniqueIdFactory.nextId('')).toStrictEqual('Custom102'); + expect(uniqueIdFactory.nextId('A')).toStrictEqual('CustomA101'); }); }); diff --git a/src/utilities/unique-id/unique-id-factory.ts b/src/utilities/unique-id/unique-id-factory.ts index 836a19ce79f..147ef3641c3 100644 --- a/src/utilities/unique-id/unique-id-factory.ts +++ b/src/utilities/unique-id/unique-id-factory.ts @@ -21,5 +21,5 @@ export class UniqueIdFactory { export function globalIdGeneratorFactory(prefix = '') { let index = 1; - return () => `polaris-${prefix}${index++}`; + return () => `Polaris${prefix}${index++}`; }