Skip to content
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

chore: support for sticky params in URL and intent ops #7429

Merged
merged 8 commits into from
Sep 2, 2024

Conversation

jordanl17
Copy link
Member

@jordanl17 jordanl17 commented Aug 29, 2024

Description

Adds ability to define sticky search params which will be retained state-fully in the URL throughout navigation

What to review

Primarily packages/sanity/src/router/RouterProvider.tsx is where the handling and persistence of the params is handled

Testing

Notes for release

N/A

Copy link

vercel bot commented Aug 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
page-building-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 30, 2024 2:37pm
performance-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 30, 2024 2:37pm
test-compiled-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 30, 2024 2:37pm
test-next-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 30, 2024 2:37pm
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 30, 2024 2:37pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Aug 30, 2024 2:37pm

Copy link
Contributor

No changes to documentation

Copy link
Contributor

github-actions bot commented Aug 29, 2024

Component Testing Report Updated Aug 30, 2024 2:33 PM (UTC)

✅ All Tests Passed -- expand for details
File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 45s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 8s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 29s 6 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 37s 11 7 0
formBuilder/inputs/PortableText/copyPaste/CopyPasteFields.spec.tsx ✅ Passed (Inspect) 0s 0 12 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 17s 6 0 0
formBuilder/inputs/PortableText/DisableFocusAndUnset.spec.tsx ✅ Passed (Inspect) 10s 3 0 0
formBuilder/inputs/PortableText/DragAndDrop.spec.tsx ✅ Passed (Inspect) 3m 0s 0 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 46s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 1m 47s 21 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 15s 18 0 0
formBuilder/inputs/PortableText/PresenceCursors.spec.tsx ✅ Passed (Inspect) 8s 3 9 0
formBuilder/inputs/PortableText/RangeDecoration.spec.tsx ✅ Passed (Inspect) 25s 9 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 17s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 34s 12 0 0
formBuilder/tree-editing/TreeEditing.spec.tsx ✅ Passed (Inspect) 0s 0 3 0
formBuilder/tree-editing/TreeEditingNestedObjects.spec.tsx ✅ Passed (Inspect) 0s 0 3 0

@jordanl17 jordanl17 changed the title Chore/corel sticky params chore: support for sticky params in URL and intent ops Aug 29, 2024
)

return <RouterContext.Provider value={router}>{props.children}</RouterContext.Provider>
}
const STICKY: string[] = []
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently there are no sticky params defined, however in content releases, the perspective key will be added here (the first current use of sticky params), but they can be used in other instances also

@juice49
Copy link
Contributor

juice49 commented Aug 30, 2024

@RitaDias @jordanl17 we have some test coverage for this behaviour in packages/sanity/src/router/IntentLink.test.tsx. However, I forgot that this branch omits all of the sticky parameters from the STICKY constant, so these tests currently fail (the tests assume perspective is sticky).

We can't test this behaviour manually or automatically while STICKY is empty. Wondering about the best way to proceed.

Any objections to making perspective sticky in this branch, even though it is currently unused?

@@ -30,4 +31,68 @@ describe('IntentLink', () => {
'/test/intent/edit/id=document-id-123;type=document-type/?perspective=bundle.summer-drop',
)
})

it('should preserve sticky parameters when resolving intent link', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great tests @juice49

bjoerge
bjoerge previously approved these changes Aug 30, 2024
Copy link
Member

@bjoerge bjoerge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, LGTM

@jordanl17 jordanl17 dismissed stale reviews from bjoerge and pedrobonamin via e89d3d0 August 30, 2024 14:18
@jordanl17
Copy link
Member Author

@juice49 @RitaDias - stick params test updated. Now the stick parameters are defined separately and are mocked in the IntentLink tests

@@ -2,6 +2,7 @@ import {fromPairs, partition, toPairs} from 'lodash'
import {type ReactElement, type ReactNode, useCallback, useMemo} from 'react'
import {RouterContext} from 'sanity/_singletons'

import {STICKY_PARAMS} from './stickyParams'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good rename 👍.

Copy link
Contributor

@juice49 juice49 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for refactoring STICKY_PARAMS to improve testability. Looks good to me!

@jordanl17 jordanl17 added this pull request to the merge queue Sep 2, 2024
Merged via the queue into next with commit 8bc721b Sep 2, 2024
42 checks passed
@jordanl17 jordanl17 deleted the chore/corel-sticky-params branch September 2, 2024 14:36
jordanl17 added a commit that referenced this pull request Sep 10, 2024
github-merge-queue bot pushed a commit that referenced this pull request Sep 10, 2024
RitaDias added a commit that referenced this pull request Oct 4, 2024
…QueryResult (#7587)

### Description

- Re-adds `QueryResult` interface
- Changing the type in the collate to not explode when versions are
present.

When rebasing against `next` something went wrong specifically around
this [PR](https://github.com/sanity-io/sanity/pull/7555/files). I wasn't
able to fix it by using rebasing however this unblocks us and allows us
to continue work.

I imagine that the fix will likely be more around making sure that the
collate prop type stayed the same (making the changes in the
ENABLE_LRU_MEMO changes however I don't know the larger impact of making
that change, so, for now I've set this up.

Folks that are more knowledgeable around this should weight in before I
merge since this is more like a temporary bandaid than a fix 🥲

Ash set up a branch `corel-pre-rebase-20241003` in case if you want to
give the rebase another stab to avoid some of these issues. Keep in mind
that you'll need to cherry pick
[StickParams](#7429) commits as
well
juice49 added a commit that referenced this pull request Oct 4, 2024
juice49 added a commit that referenced this pull request Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants