useNavigationCurrentEntry customer account #1526
Conversation
| import {useApi} from './api'; | ||
| import {useEffect, useReducer} from 'react'; | ||
|
|
||
| export function useLiveFullPageNavigation< |
There was a problem hiding this comment.
Extension points without full page target cant use this hook
There was a problem hiding this comment.
Can we throw an error if it's used in an unsupported extension point?
There was a problem hiding this comment.
Just did! If its a standard navigation, we wont have the current entry and the navigation events so throwing an error in that case
| import {useApi} from './api'; | ||
| import {useEffect, useReducer} from 'react'; | ||
|
|
||
| export function useLiveFullPageNavigation< |
There was a problem hiding this comment.
Can we add tests for this? 🤔
There was a problem hiding this comment.
I know there's gonna be some mocking required, but we can ensure that the state is synchronized with the currententrychange event.
There was a problem hiding this comment.
Updated the tests
This comment has been minimized.
This comment has been minimized.
| import {useApi} from './api'; | ||
| import {useEffect, useReducer} from 'react'; | ||
|
|
||
| export function useLiveFullPageNavigation< |
There was a problem hiding this comment.
I think we never use the word live anywhere for subscriptions.
Can we rename this hook to maybe useNavigationCurrentEntry? I feel like useCurrentEntry could also work, but might be unclear on it's own? 🤔
There was a problem hiding this comment.
I think I prefer useNavigationCurrentEntry its more descriptive that it belongs to the navigation api
There was a problem hiding this comment.
This would require some coordination, but can we move this hook into navigation after this PR is merged?
| useEffect(() => { | ||
| if (!currentEntry || !removeEventListener || !addEventListener) { | ||
| throw new Error( | ||
| 'useLiveFullPageNavigation must be used in a full page extension only', |
There was a problem hiding this comment.
Let's include the actual target name so it's easier to google?
|
|
||
| destroyAll(); | ||
|
|
||
| expect(mock.removeEventListener).toHaveBeenCalledWith( |
There was a problem hiding this comment.
mock isn't being reset after/before each test, right?
So if you added another test that asserted on mock.removeEventListener to have been called this would always be true even if it was never called inside the test, right?
Could we maybe put the mock into a createMock function that we call in each test? This way, it'd always be a new instance.
| const {currentEntry, removeEventListener, addEventListener} = | ||
| useApi<Target>().navigation; | ||
|
|
||
| const [entry, update] = useReducer(() => currentEntry, currentEntry); |
There was a problem hiding this comment.
Is this the implementation from the RFC comment? :D
There was a problem hiding this comment.
Its pretty close yeah! haha
oluwatimio
changed the title
Background
Partially Fixes https://github.com/Shopify/core-issues/issues/62603
Solution
🎩
Tophat https://github.com/Shopify/customer-account-web/pull/3400
Checklist