-
Notifications
You must be signed in to change notification settings - Fork 457
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
refactor(preview): clean up preview with fallback logic #8904
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
ee7beb1
to
e2564a1
Compare
No changes to documentation |
⚡️ Editor Performance ReportUpdated Wed, 12 Mar 2025 17:49:19 GMT
Detailed information🏠 Reference resultThe performance result of
🧪 Experiment resultThe performance result of this branch
📚 Glossary
|
@@ -14,25 +13,31 @@ export function getMissingDocumentFallback(document: Partial<SanityDocument> | P | |||
} | |||
} | |||
|
|||
export type Values = { |
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.
Not very satisfied with the name of this type 🙄 suggestions appreciated
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.
PreviewSources
?
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.
I like that! Aligns with the source
parameter(s) of Object.assign
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.
Fixed in 4653034, thank you!
@@ -84,7 +84,7 @@ export function PaneItemPreview(props: PaneItemPreviewProps) { | |||
|
|||
return ( | |||
<SanityDefaultPreview | |||
{...getPreviewValueWithFallback({document, snapshot, original})} | |||
{...getPreviewValueWithFallback({snapshot, original, fallback: value})} |
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.
previously document
would refer to the global document here 🙈
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.
Great catch! Thank you! 🙇
* last resort fallback in case we don't have anything to preview | ||
* this can be a hard-coded preview value, or a document stub | ||
*/ | ||
fallback?: Partial<SanityDocument> | PreviewValue |
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.
hoping this makes it slightly more clear that you can pass the document and/or a hard coded preview stub as fallback
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 a nice improvement. I wondered whether it might be even better to use a union to ensure at least one value is passed, but upon further consideration, I'm not sure that's possible or desired.
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.
Thought about it briefly, but that would be a union of three objects, each with one of these keys required and the other two optional, right? I concluded the added complexity wasn't justified, but happy to reconsider (but I'd prefer a follow-up PR for that).
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.
I agree with you.
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.
Thanks for tackling this! It looks good to me when testing in browser.
Description
Fixes a couple of issues:
1. Search previews didn't reflect current perspective
Before
After
2. Document list previews didn't reflect current perspective
Before
After
What to review
Took the opportunity to clean up and document the logic related to getting preview values and fallback
Also fixed a bug that leaked document.title in previews for cases where the document was missing.
Testing
Switching between release/draft/published should use the correct preview for search results (global search, reference search), document lists etc.
Notes for release