Allow id to be null #1633
Allow id to be null #1633
Conversation
size-limit report 📦
|
There was a problem hiding this comment.
If ID is null it won't do anything which makes it weird that you'd call it in the first place. How about we create a second export that makes all of this available as a non-hook? Based on the logic that seems to be possible and easy enough to do?
Alternatively you could export getEventName() and let consumers implement custom logic that does the same thing but outside of a hook
envex
force-pushed
the
envex/use-watch-null-id
branch
from
bcf9e6a to
2fd3dd6
Compare
| @@ -7,18 +7,34 @@ import {getEventName} from './ColorVisionA11y'; | |||
| import {useCallbackRef} from './useCallbackRef'; | |||
|
|
|||
| export function useWatchActiveSeries( | |||
| id: string, | |||
| id: string | null, | |||
There was a problem hiding this comment.
The point of adding the separate setActiveSeriesListener() is so we don't need to make this optional right? So let's remove that logic?
packages/polaris-viz/src/hooks/tests/useWatchActiveSeries.test.tsx
Outdated
Show resolved
Hide resolved
packages/polaris-viz/src/hooks/tests/useWatchActiveSeries.test.tsx
Outdated
Show resolved
Hide resolved
packages/polaris-viz/src/hooks/tests/useWatchActiveSeries.test.tsx
Outdated
Show resolved
Hide resolved
envex
force-pushed
the
envex/use-watch-null-id
branch
from
2fd3dd6 to
db39619
Compare
| describe('useWatchActiveSeries()', () => { | ||
| describe('useWatchActiveSeries()', () => { | ||
| it('attaches event listeners to window when id is passed', () => { | ||
| const addEventListenerSpy = jest.spyOn(window, 'addEventListener'); | ||
|
|
||
| function MockComponent() { | ||
| useWatchActiveSeries('id', () => {}); | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| mount(<MockComponent />); | ||
|
|
||
| expect(addEventListenerSpy).toHaveBeenCalledWith( | ||
| 'id:color-vision-event:singleItem', | ||
| expect.any(Function), | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('setActiveSeriesListener()', () => { | ||
| it('generates an event name using the given ID', () => { | ||
| const id = getEventName('someChartId', COLOR_VISION_SINGLE_ITEM); | ||
| const addEventListenerSpy = jest.spyOn(window, 'addEventListener'); | ||
|
|
||
| setActiveSeriesListener('someChartId', () => {}); | ||
|
|
||
| expect(addEventListenerSpy).toHaveBeenCalledWith( | ||
| id, | ||
| expect.any(Function), | ||
| ); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
There seem to be tests missing here. E.g. tests that make sure the listener gets removed on unmount
There was a problem hiding this comment.
@kvendrik Added a test for removeEventListener, let me know if you think it makes sense to add one to ensure setActiveSeriesListener returns a function as well.
envex
force-pushed
the
envex/use-watch-null-id
branch
from
db39619 to
bd09112
Compare
| }); | ||
|
|
||
| describe('setActiveSeriesListener()', () => { | ||
| it('generates an event name using the given ID', () => { |
There was a problem hiding this comment.
| it('generates an event name using the given ID', () => { | |
| it('adds en event listener using an event name generated using the given ID', () => { |
| id, | ||
| expect.any(Function), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Missing test: returns a method that removes the previously set event listener
| const id = getEventName('someChartId', COLOR_VISION_SINGLE_ITEM); | ||
| const addEventListenerSpy = jest.spyOn(window, 'addEventListener'); | ||
|
|
||
| setActiveSeriesListener('someChartId', () => {}); |
There was a problem hiding this comment.
Test should also make sure that the given callback is the one that's being used for the addEventListener call
envex
force-pushed
the
envex/use-watch-null-id
branch
from
bd09112 to
ade1ce1
Compare
envex
force-pushed
the
envex/use-watch-null-id
branch
from
ade1ce1 to
7f38db2
Compare
What does this implement/fix?
Allows a consumer to conditionally watch for events. If for some reason the
idprop is not available, we won't attach the events to window.