-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(next-app-router) implement an adapter for next-app-router #75
base: main
Are you sure you want to change the base?
Conversation
}, | ||
"peerDependencies": { | ||
"next": ">=13", | ||
"react": ">=16.8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"react": ">=16.8" | |
"react": ">=18.2" |
Next.js version 13 required react version 18.2.0, So it would be nice to version this as well.
window.history.go(index); | ||
}, | ||
}), | ||
[history, currentIndex, currentState, searchParams, id, state], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[history, currentIndex, currentState, searchParams, id, state], | |
[history, currentIndex, currentState, searchParams, id], |
I think history.state is not to need createUseFunnel
result. It seems that currentIndex
, currentState
, history
is enough to construct the result.
const newHistoryState = { | ||
...state, | ||
[`${id}.context`]: newState.context, | ||
[`${id}.histories`]: [...(history ?? []), newState], | ||
}; | ||
|
||
newSearchParams.set(`${id}.step`, newState.step); | ||
window.history.pushState(newHistoryState, '', `?${newSearchParams.toString()}`); | ||
setState(newHistoryState); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const newHistoryState = { | |
...state, | |
[`${id}.context`]: newState.context, | |
[`${id}.histories`]: [...(history ?? []), newState], | |
}; | |
newSearchParams.set(`${id}.step`, newState.step); | |
window.history.pushState(newHistoryState, '', `?${newSearchParams.toString()}`); | |
setState(newHistoryState); | |
newSearchParams.set(`${id}.step`, newState.step); | |
window.history.pushState(newHistoryState, '', `?${newSearchParams.toString()}`); | |
setState(prevHistoryState => ({ | |
...prevHistoryState, | |
[`${id}.context`]: newState.context, | |
[`${id}.histories`]: [...(history ?? []), newState], | |
})); |
Instead of removing the dependency on history.state
in useMemo, I think we could use something like this.
Description
Implement an adapter for next-app-router for the issue discussed in #59.
Related Issue : #59
Context
Currently implemented to operate similarly to @use-funnel/browser. Used for history.state. context, history
useSearchParams in next.js requires suspense, you must wrap suspense when using useFunnel.
How Has This Been Tested
I tested locally whether the value was maintained through history.state even when refreshed.
I checked in the local environment whether the funnel steps were changed appropriately even when using the browser's back and forward functions.
Added test case for funnel.render.overlay.
I tested whether pages using useFunnel could be built without problems.
Further Comments
I look forward to app router users being able to use use-funnel too!
If there is anything else that needs correction, please let me know at any time.
thank you