Skip to content

Commit 7669582

Browse files
BPScottBPScott
committed
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
1 parent 76a7808 commit 7669582

File tree

4 files changed

+80
-66
lines changed

4 files changed

+80
-66
lines changed

‎src/utilities/unique-id/hooks.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,19 +2,19 @@ import {useContext, useRef} from 'react';
22
import {UniqueIdFactoryContext} from './context';
33

44
/**
5-
* Returns a unique id that remains consistent across multiple rerenders of the
5+
* Returns a unique id that remains consistent across multiple re-renders of the
66
* same hook
77
* @param prefix Defines a prefix for the ID. You probably want to set this to
88
* the name of the component you're calling `useUniqueId` in.
99
* @param overrideId Defines a fixed value to use instead of generating a unique
10-
* ID. Useful for components that allow consumers to specify a fixed ID.
10+
* ID. Useful for components that allow consumers to specify their own ID.
1111
*/
1212
export function useUniqueId(prefix = '', overrideId = '') {
1313
const idFactory = useContext(UniqueIdFactoryContext);
1414

15-
// By using a ref to store the uniqueId for each incovation of the hook and
15+
// By using a ref to store the uniqueId for each invocation of the hook and
1616
// checking that it is not already populated we ensure that we don't generate
17-
// a new ID on ever rerender of a component.
17+
// a new ID on every re-render of a component.
1818
const uniqueIdRef = useRef<string | null>(null);
1919

2020
if (!idFactory) {

‎src/utilities/unique-id/tests/hooks.test.tsx

Lines changed: 62 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,14 @@ function TestHarness({children}: {children: React.ReactNode}) {
66
return <React.Fragment>{children}</React.Fragment>;
77
}
88

9-
const Component1 = () => <React.Fragment>{useUniqueId()}</React.Fragment>;
10-
const Component2 = () => <React.Fragment>{useUniqueId()}</React.Fragment>;
11-
const Component3 = () => <React.Fragment>{useUniqueId()}</React.Fragment>;
9+
const Component1 = () => <div id={useUniqueId()} />;
10+
const Component2 = () => <div id={useUniqueId()} />;
11+
const Component3 = () => <div id={useUniqueId()} />;
1212

13-
const HasPrefix1 = () => <React.Fragment>{useUniqueId('a')}</React.Fragment>;
14-
const HasPrefix2 = () => <React.Fragment>{useUniqueId('a')}</React.Fragment>;
15-
const HasPrefix3 = () => <React.Fragment>{useUniqueId('a')}</React.Fragment>;
16-
const HasPrefix4 = () => <React.Fragment>{useUniqueId('b')}</React.Fragment>;
13+
const HasPrefix1 = () => <div id={useUniqueId('A')} />;
14+
const HasPrefix2 = () => <div id={useUniqueId('A')} />;
15+
const HasPrefix3 = () => <div id={useUniqueId('A')} />;
16+
const HasPrefix4 = () => <div id={useUniqueId('B')} />;
1717

1818
describe('useUniqueId', () => {
1919
it('returns unique IDs across a single component', () => {
@@ -28,13 +28,12 @@ describe('useUniqueId', () => {
2828
</TestHarness>,
2929
);
3030

31-
expect(harness.findAll(Component1)[0].html()).toStrictEqual('polaris-1');
32-
expect(harness.findAll(Component1)[1].html()).toStrictEqual('polaris-2');
33-
expect(harness.findAll(Component1)[2].html()).toStrictEqual('polaris-3');
34-
35-
expect(harness.findAll(HasPrefix1)[0].html()).toStrictEqual('polaris-a1');
36-
expect(harness.findAll(HasPrefix1)[1].html()).toStrictEqual('polaris-a2');
37-
expect(harness.findAll(HasPrefix1)[2].html()).toStrictEqual('polaris-a3');
31+
expect(harness.findAll('div')[0]).toHaveReactProps({id: 'Polaris1'});
32+
expect(harness.findAll('div')[1]).toHaveReactProps({id: 'Polaris2'});
33+
expect(harness.findAll('div')[2]).toHaveReactProps({id: 'Polaris3'});
34+
expect(harness.findAll('div')[3]).toHaveReactProps({id: 'PolarisA1'});
35+
expect(harness.findAll('div')[4]).toHaveReactProps({id: 'PolarisA2'});
36+
expect(harness.findAll('div')[5]).toHaveReactProps({id: 'PolarisA3'});
3837
});
3938

4039
it('returns unique IDs across multiple components', () => {
@@ -50,41 +49,39 @@ describe('useUniqueId', () => {
5049
</TestHarness>,
5150
);
5251

53-
expect(harness.find(Component1)!.html()).toStrictEqual('polaris-1');
54-
expect(harness.find(Component2)!.html()).toStrictEqual('polaris-2');
55-
expect(harness.find(Component3)!.html()).toStrictEqual('polaris-3');
56-
57-
expect(harness.find(HasPrefix1)!.html()).toStrictEqual('polaris-a1');
58-
expect(harness.find(HasPrefix2)!.html()).toStrictEqual('polaris-a2');
59-
expect(harness.find(HasPrefix3)!.html()).toStrictEqual('polaris-a3');
60-
expect(harness.find(HasPrefix4)!.html()).toStrictEqual('polaris-b1');
52+
expect(harness.findAll('div')[0]).toHaveReactProps({id: 'Polaris1'});
53+
expect(harness.findAll('div')[1]).toHaveReactProps({id: 'Polaris2'});
54+
expect(harness.findAll('div')[2]).toHaveReactProps({id: 'Polaris3'});
55+
expect(harness.findAll('div')[3]).toHaveReactProps({id: 'PolarisA1'});
56+
expect(harness.findAll('div')[4]).toHaveReactProps({id: 'PolarisA2'});
57+
expect(harness.findAll('div')[5]).toHaveReactProps({id: 'PolarisA3'});
58+
expect(harness.findAll('div')[6]).toHaveReactProps({id: 'PolarisB1'});
6159
});
6260

6361
it('increments multiple IDs within the same component', () => {
6462
const HasMultipleIds = () => (
65-
<React.Fragment>
66-
{useUniqueId()} :: {useUniqueId()}
67-
</React.Fragment>
63+
<div id={useUniqueId()} title={useUniqueId()} />
6864
);
6965

7066
const harness = mountWithApp(<HasMultipleIds />);
7167

72-
expect(harness.html()).toStrictEqual('polaris-1 :: polaris-2');
68+
expect(harness.find('div')).toHaveReactProps({
69+
id: 'Polaris1',
70+
title: 'Polaris2',
71+
});
7372
});
7473

7574
it('uses an override if specified', () => {
76-
const HasOverride = () => (
77-
<React.Fragment>{useUniqueId('', 'overridden')}</React.Fragment>
78-
);
75+
const HasOverride = () => <div id={useUniqueId('', 'overridden')} />;
7976

8077
const harness = mountWithApp(<HasOverride />);
8178

82-
expect(harness.html()).toStrictEqual('overridden');
79+
expect(harness.find('div')).toHaveReactProps({id: 'overridden'});
8380
});
8481

8582
it('uses an override if specified and the override does not interupt the count', () => {
8683
const HasOverride = ({idOverride}: {idOverride?: string}) => (
87-
<React.Fragment>{useUniqueId('', idOverride)}</React.Fragment>
84+
<div id={useUniqueId('', idOverride)} />
8885
);
8986

9087
const harness = mountWithApp(
@@ -95,16 +92,14 @@ describe('useUniqueId', () => {
9592
</TestHarness>,
9693
);
9794

98-
expect(harness.findAll(HasOverride)[0].html()).toStrictEqual('polaris-1');
99-
expect(harness.findAll(HasOverride)[1].html()).toStrictEqual('overridden');
100-
expect(harness.findAll(HasOverride)[2].html()).toStrictEqual('polaris-2');
95+
expect(harness.findAll('div')[0]).toHaveReactProps({id: 'Polaris1'});
96+
expect(harness.findAll('div')[1]).toHaveReactProps({id: 'overridden'});
97+
expect(harness.findAll('div')[2]).toHaveReactProps({id: 'Polaris2'});
10198
});
10299

103100
it('keeps the same ID across multiple rerenders', () => {
104101
const HasProp = ({info}: {info: string}) => (
105-
<React.Fragment>
106-
{info} :: {useUniqueId()}
107-
</React.Fragment>
102+
<div id={useUniqueId()} title={info} />
108103
);
109104

110105
const ReRenderingTestHarness = () => {
@@ -124,19 +119,23 @@ describe('useUniqueId', () => {
124119

125120
const harness = mountWithApp(<ReRenderingTestHarness />);
126121

127-
expect(harness.find(HasProp)!.html()).toStrictEqual('count1 :: polaris-1');
122+
expect(harness.findAll('div')[0]).toHaveReactProps({
123+
title: 'count1',
124+
id: 'Polaris1',
125+
});
128126

129127
harness.find('button')!.trigger('onClick');
130128

131-
expect(harness.find(HasProp)!.html()).toStrictEqual('count2 :: polaris-1');
129+
expect(harness.findAll('div')[0]).toHaveReactProps({
130+
title: 'count2',
131+
id: 'Polaris1',
132+
});
132133
});
133134

134135
it('updates the ID if the overridden ID changes', () => {
135136
type HasPropProps = {info: string; idOverride?: string};
136137
const HasProp = ({info, idOverride}: HasPropProps) => (
137-
<React.Fragment>
138-
{info} :: {useUniqueId('', idOverride)}
139-
</React.Fragment>
138+
<div id={useUniqueId('', idOverride)} title={info} />
140139
);
141140

142141
const ReRenderingTestHarness = () => {
@@ -147,7 +146,7 @@ describe('useUniqueId', () => {
147146
);
148147

149148
// eslint-disable-next-line shopify/jest/no-if
150-
const override = count % 2 === 0 ? `override-${count}` : undefined;
149+
const override = count % 2 === 0 ? `Override${count}` : undefined;
151150

152151
return (
153152
<React.Fragment>
@@ -160,23 +159,38 @@ describe('useUniqueId', () => {
160159
const harness = mountWithApp(<ReRenderingTestHarness />);
161160

162161
// Initially we use an incremental id
163-
expect(harness.find(HasProp)!.html()).toStrictEqual('count1 :: polaris-1');
162+
expect(harness.find('div')).toHaveReactProps({
163+
title: 'count1',
164+
id: 'Polaris1',
165+
});
164166

165167
// But then we set an override id, so it should use that
166168
harness.find('button')!.trigger('onClick');
167-
expect(harness.find(HasProp)!.html()).toStrictEqual('count2 :: override-2');
169+
expect(harness.find('div')).toHaveReactProps({
170+
title: 'count2',
171+
id: 'Override2',
172+
});
168173

169174
// Then on the next render we don't set an override id, so we should go back
170175
// to using the incremental id
171176
harness.find('button')!.trigger('onClick');
172-
expect(harness.find(HasProp)!.html()).toStrictEqual('count3 :: polaris-1');
177+
expect(harness.find('div')).toHaveReactProps({
178+
title: 'count3',
179+
id: 'Polaris1',
180+
});
173181

174182
// Back to setting an override id
175183
harness.find('button')!.trigger('onClick');
176-
expect(harness.find(HasProp)!.html()).toStrictEqual('count4 :: override-4');
184+
expect(harness.find('div')).toHaveReactProps({
185+
title: 'count4',
186+
id: 'Override4',
187+
});
177188

178189
// Back to not setting an override, so back to using the incremental id
179190
harness.find('button')!.trigger('onClick');
180-
expect(harness.find(HasProp)!.html()).toStrictEqual('count5 :: polaris-1');
191+
expect(harness.find('div')).toHaveReactProps({
192+
title: 'count5',
193+
id: 'Polaris1',
194+
});
181195
});
182196
});

‎src/utilities/unique-id/tests/unique-id-factory.test.ts

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,33 +4,33 @@ describe('UniqueIdFactory', () => {
44
it('returns unique IDs across multiple calls', () => {
55
const uniqueIdFactory = new UniqueIdFactory(globalIdGeneratorFactory);
66

7-
expect(uniqueIdFactory.nextId('')).toStrictEqual('polaris-1');
8-
expect(uniqueIdFactory.nextId('')).toStrictEqual('polaris-2');
9-
expect(uniqueIdFactory.nextId('')).toStrictEqual('polaris-3');
7+
expect(uniqueIdFactory.nextId('')).toStrictEqual('Polaris1');
8+
expect(uniqueIdFactory.nextId('')).toStrictEqual('Polaris2');
9+
expect(uniqueIdFactory.nextId('')).toStrictEqual('Polaris3');
1010

11-
expect(uniqueIdFactory.nextId('a')).toStrictEqual('polaris-a1');
12-
expect(uniqueIdFactory.nextId('a')).toStrictEqual('polaris-a2');
13-
expect(uniqueIdFactory.nextId('a')).toStrictEqual('polaris-a3');
11+
expect(uniqueIdFactory.nextId('A')).toStrictEqual('PolarisA1');
12+
expect(uniqueIdFactory.nextId('A')).toStrictEqual('PolarisA2');
13+
expect(uniqueIdFactory.nextId('A')).toStrictEqual('PolarisA3');
1414
});
1515

1616
it('returns unique IDs across prefixes', () => {
1717
const uniqueIdFactory = new UniqueIdFactory(globalIdGeneratorFactory);
1818

19-
expect(uniqueIdFactory.nextId('a')).toStrictEqual('polaris-a1');
20-
expect(uniqueIdFactory.nextId('a')).toStrictEqual('polaris-a2');
21-
expect(uniqueIdFactory.nextId('a')).toStrictEqual('polaris-a3');
19+
expect(uniqueIdFactory.nextId('A')).toStrictEqual('PolarisA1');
20+
expect(uniqueIdFactory.nextId('A')).toStrictEqual('PolarisA2');
21+
expect(uniqueIdFactory.nextId('A')).toStrictEqual('PolarisA3');
2222
});
2323

2424
it('can accept a custom factory', () => {
2525
const customIdGeneratorFactory = (prefix: string) => {
2626
let index = 101;
27-
return () => `custom-${prefix}${index++}`;
27+
return () => `Custom${prefix}${index++}`;
2828
};
2929

3030
const uniqueIdFactory = new UniqueIdFactory(customIdGeneratorFactory);
3131

32-
expect(uniqueIdFactory.nextId('')).toStrictEqual('custom-101');
33-
expect(uniqueIdFactory.nextId('')).toStrictEqual('custom-102');
34-
expect(uniqueIdFactory.nextId('a')).toStrictEqual('custom-a101');
32+
expect(uniqueIdFactory.nextId('')).toStrictEqual('Custom101');
33+
expect(uniqueIdFactory.nextId('')).toStrictEqual('Custom102');
34+
expect(uniqueIdFactory.nextId('A')).toStrictEqual('CustomA101');
3535
});
3636
});

‎src/utilities/unique-id/unique-id-factory.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,5 +21,5 @@ export class UniqueIdFactory {
2121

2222
export function globalIdGeneratorFactory(prefix = '') {
2323
let index = 1;
24-
return () => `polaris-${prefix}${index++}`;
24+
return () => `Polaris${prefix}${index++}`;
2525
}

0 commit comments

Comments
 (0)