refactor: migrate @sanity/presentation codebase
#8312
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@sanity/[email protected] |
stipsan
force-pushed
the
migrate-presentation
branch
from
2ca06a3 to
9078d45
Compare
stipsan
force-pushed
the
migrate-presentation
branch
from
9078d45 to
d8923d4
Compare
|
No changes to documentation |
|
Component Testing Report Updated Jan 17, 2025 6:29 PM (UTC) ❌ Failed Tests (1) -- expand for details
|
⚡️ Editor Performance ReportUpdated Fri, 17 Jan 2025 18:31:28 GMT
Detailed information🏠 Reference resultThe performance result of
🧪 Experiment resultThe performance result of this branch
📚 Glossary
|
stipsan
force-pushed
the
migrate-presentation
branch
from
d8923d4 to
4f05526
Compare
stipsan
force-pushed
the
migrate-presentation
branch
from
4f05526 to
e1e42ba
Compare
stipsan
force-pushed
the
migrate-presentation
branch
from
e1e42ba to
1120ad8
Compare
stipsan
force-pushed
the
migrate-presentation
branch
from
1120ad8 to
884967a
Compare
stipsan
force-pushed
the
migrate-presentation
branch
from
6cb220a to
1aaf5c4
Compare
stipsan
force-pushed
the
migrate-presentation
branch
from
1aaf5c4 to
697e9a4
Compare
| } | ||
| ` | ||
|
|
||
| export const Resizer: FunctionComponent<{ |
There was a problem hiding this comment.
NIT, We have a Resizer that we could replace this one with as a next step. packages/sanity/src/core/components/resizer/Resizer.tsx
There was a problem hiding this comment.
Could we do that in a follow up PR?
There was a problem hiding this comment.
Yes, sure! Added some small suggestions for follow up, nothing is blocking.
| white-space: pre-wrap; | ||
| ` | ||
|
|
||
| export function DocumentListPane(props: { |
There was a problem hiding this comment.
NIT - follow up, could be good to rename this file to something else to avoid confusions with the structure/documentListPane
| white-space: pre-wrap; | ||
| ` | ||
|
|
||
| export function DocumentPane(props: { |
There was a problem hiding this comment.
NIT - follow up, could be good to rename this file to something else to avoid confusions with the structure/DocumentPane
| @@ -0,0 +1,12 @@ | |||
| export function debounce<F extends (...args: Parameters<F>) => ReturnType<F>>( | |||
There was a problem hiding this comment.
NIT, follow up, could be replaced for debounce from lodash
| const value = props.value as SanityDocument | ||
| const documentId = value?._id ? getPublishedId(value?._id) : undefined | ||
|
|
||
| if (isDocumentSchemaType(props.schemaType)) { |
There was a problem hiding this comment.
NIT , follow up this seems to be added only once for the document.
I think you could benefit from using unstable_layout instead of checking in each input if it's a document schema type.
| import {API_VERSION} from './constants' | ||
| import {type PreviewUrlOption} from './types' | ||
|
|
||
| export function usePreviewUrl( |
There was a problem hiding this comment.
Follow up. - we have a usePreviewURL function in structure would be nice to see if we can join the two https://github.com/sanity-io/sanity/blob/corel-references/packages/sanity/src/structure/panes/document/usePreviewUrl.ts#L11-L12
pedrobonamin
previously approved these changes
Jan 17, 2025
There was a problem hiding this comment.
Looks good to me, thanks for moving this @stipsan !
Added some small suggestions but nothing is blocking, more of some future iterations we can do.
stipsan
force-pushed
the
migrate-presentation
branch
from
697e9a4 to
7c490c6
Compare
stipsan
added a commit
that referenced
this pull request
Jan 17, 2025
stipsan
added a commit
that referenced
this pull request
Jan 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR copies over the codebase for
@sanity/presentationon itsmainbranch. I've then refactored the code to pass our linter checks for things like:React.createContextonly being allowed insanity/_singletons.ui-componentslike<Button />,<Tooltip>and such should be used instead of importing directly from@sanity/uiThe codebase is otherwise the same. It's tempting to refactor and start cleaning things up, but it's important to note that after this PR is merged, I'll have to:
corelbranchcorelbranch, on top of Presentation.packages/presentationfrom thevisual-editingmonorepo, and start adding over tests for more advanced presentation features that aren't enabled in the current test studio.It's more important that we reach the step where we can delete the code from
visual-editingASAP, so we avoid new work being done there, for example by external contributors.What to review
Is the new code following conventions ok?
It does pull in a bunch of new dependencies, and I want to work through them and get rid of unnecessary ones (we only use
suspend-reactin one instance, and it doesn't really need to). But they are lazy loaded, and isn't much different from how we re-export@sanity/presentationas-is today.Testing
Tests are lacking, once this is merged in we can start working on playwright component tests for Presentation, as well as better vitest coverage for utils and parsers.
For now the PR build seems to work like it did before the migration: https://test-studio-git-migrate-presentation.sanity.dev/presentation/presentation/simpleBlock/716e2c8a-a168-4852-87e2-0c2b4d42c94b/body%5B_key==%22161e19ce1c6b%22%5D.children%5B_key==%22af5299cd594b%22%5D.text
Notes for release
The
@sanity/presentationcodebase has been migrated into thesanitycodebase. We've always recommended using Presentation Tool by importing it fromsanity/presentationlike so:If you've been following this pattern then this change won't impact you in any way.
If you've been installing
@sanity/presentationand are using it directly (we've sometimes done this to let folks try out bugfixes before they ship in the next release ofsanity):Then you need to search/replace all
from '@sanity/presentation'statements in your codebase withfrom 'sanity/presentation', and uninstall@sanity/presentationas it'll no longer receive updates.