-
Notifications
You must be signed in to change notification settings - Fork 451
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
fix: creating new release immediately pins as perspective #8620
base: next
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
Coverage Report
File Coverage
|
No changes to documentation |
⚡️ Editor Performance ReportUpdated Mon, 17 Feb 2025 11:03:45 GMT
Detailed information🏠 Reference resultThe performance result of
🧪 Experiment resultThe performance result of this branch
📚 Glossary
|
Component Testing Report Updated Feb 17, 2025 11:08 AM (UTC) ❌ Failed Tests (3) -- expand for details
|
31ae559
to
b835735
Compare
b835735
to
c7784ec
Compare
packages/sanity/src/router/types.ts
Outdated
@@ -165,13 +165,20 @@ export type MatchResult = MatchError | MatchOk | |||
/** | |||
* @public | |||
*/ | |||
export interface NavigateOptions { | |||
export interface NavigateStickyParamsOptions { |
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.
Seems like this refactor should be the other way around? The replace
-option belongs in NavigateOptions
, not NavigateStickyParamsOptions
, no?
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.
Hmm, so the reason I did it this way round was because although it feels more intuitive that NavigateStickyParamOptions
extends from NavigateOptions
, NavigateOptions
is the only one to include options.stickyParams
.
Perhaps I can just do type NavigateStickyParamOptions = Omit<NavigateOptions, 'stickyParams'>
so it feels more natural?
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…rk as expected in navigate with stickyParam cases
9b25710
to
bedeabd
Compare
Description
A PR of 2 halves: 1 simple, 1 more involved:
Simple:
For pinning the new release when created from the perspective menu, or from the 'copy to new release' flow, it's possible to just
setPerspective
to assign theperspective
sticky param. This is because we don't navigate elsewhere when a release is created in these flows.More:
In the other flow where a release can be created - from 'create release' CTA in releases overview, we already navigate to the release detail page after creation.
This causes some issues - calling
router.navigateStickyParams
(which is used bysetPerspective
), followed immediately byrouter.navigate
to navigate to the release detail, causes a race, wherenavigate
knows nothing about the perspectivestickyParam
, so the end result is that the release is not pinned, but you are still navigated to the release details.One option could be to resolve this by flushing the event loop and making
navigateStickyParams
async, but that feels rather too hacky.This PR takes the approach of extending
navigate
in therouter
. It's now possible to provide anoptions.stickyParams
object, which is much the same as the argument already passed tonavigateStickyParams
. Doing this means thatnavigate
can both update the router state and params together.This perhaps raises a question around whether this approach could deprecate
navigateStickyParams
- but givennavigateStickyParams
is already in the wild, perhaps it's best we keep it around.What to review
router.navigate
make senseTesting
Manually verified that pinning works in all places where a new release can be created
Also added tests for
RouterProvider
given the adjustments there. Ran these tests against thenext
RouterProvider
(only the relevant ones given nostickyParam
support fornavigate
innext
, and verified no regressions to functionalityNotes for release
navigate
fromuseRouter
now supports astickyParams
option to allow for navigating and updating param state in the studio at once.navigateStickyParams
is deprecated fromsanity/router
- instead usenavigate
as follows:Additionally, it's now possible to update route state and set
stickyParams
together: