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

refactor(desk-tool): up TS coverage; introduce pane types #2853

Merged
merged 6 commits into from
Oct 18, 2021

Conversation

ricokahler
Copy link
Contributor

@ricokahler ricokahler commented Oct 13, 2021

Description

The following PR:

  • Moves files out of lib folders — kind of a small change but we have ignore files at the top-level of the monorepo that target **/lib. I think by convention we shouldn't use lib folder in src (similar to how we shouldn't use dist folders inside of src).
  • Hoists the desk-tool types to @sanity/types — The idea behind this move was to make the types for these pane items more accessible and enforceable. The end result is much more descriptive types internally to the desk-tool which should also make it more portable for the future. Eventually these types should be enforced inside of @sanity/structure but that can come at a later time (I did try and it seems too difficult to combine due to many slight type nuances). This was moved to /src/desk-tool/types instead
  • Deletes @sanity/base/__legacy/@sanity/components/menu/types — after carefully looking the MenuItem type, I came to the conclusion the type is a desk-tool-only concept and renamed it PaneMenuItem and PaneMenuItemGroup. These types now live in @sanity/types/src/desk-tool and gets us really close to removing the @sanity/base/__legacy/@sanity/components folder 😄 . These types were kept and base was not touched
  • Add types to resolvePanes and more — and also removes the stub StructurePane type
  • Removes noImplicitAny from the tsconfig 🎉

TODO:

  • move new types to desk-tool
  • rename PaneXs to PaneXNode etc
  • put back legacy types that don't touch legacy directory

What to review

Review #2852 before this one.

Most of the changes are renames and adding types so the review should go quicker than what it seems. Review the new types, the names, and their usage. There was no intention on changing any logic in this one so keep an eye out of anything divergent.

@vercel
Copy link

vercel bot commented Oct 13, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

perf-studio – ./

🔍 Inspect: https://vercel.com/sanity-io/perf-studio/AzjnuDTAJ1nChKAR7km44hwHqXZi
✅ Preview: https://perf-studio-git-refactor-desk-tool-ts-coverage.sanity.build

test-studio – ./

🔍 Inspect: https://vercel.com/sanity-io/test-studio/EnLeABUfrbtjXbDEpLQyHaf1CRt9
✅ Preview: https://test-studio-git-refactor-desk-tool-ts-coverage.sanity.build

studio-workshop – ./dev/workshop

🔍 Inspect: https://vercel.com/sanity-io/studio-workshop/4cKRFqhUVZL3RsC4q8YifePvBSJd
✅ Preview: https://studio-workshop-git-refactor-desk-tool-ts-coverage.sanity.build

@mariuslundgard
Copy link
Member

Hoists the desk-tool types to @sanity/types — The idea behind this move was to make the types for these pane items more accessible and enforceable. The end result is much more descriptive types internally to the desk-tool which should also make it more portable for the future. Eventually these types should be enforced inside of @sanity/structure but that can come at a later time (I did try and it seems too difficult to combine due to many slight type nuances).

I think the best is to keep these within @sanity/desk-tool for now. The concept of "panes" is very specific to desk-tool.

Another thought I had when reading these changes, was that I think the Pane interface should be named PaneNode or similar. The reason is that we already have a UI component in desk tool named Pane.

Base automatically changed from chore/use-ts-noshadow to next October 13, 2021 13:48
@ricokahler ricokahler marked this pull request as draft October 13, 2021 14:24
@ricokahler ricokahler force-pushed the refactor/desk-tool-ts-coverage branch from 2d7a12b to 1910764 Compare October 13, 2021 19:12
@ricokahler ricokahler force-pushed the refactor/desk-tool-ts-coverage branch from 1910764 to cf9b62b Compare October 13, 2021 19:16
@ricokahler ricokahler marked this pull request as ready for review October 13, 2021 19:17
@ricokahler ricokahler force-pushed the refactor/desk-tool-ts-coverage branch from cf9b62b to fd77ed0 Compare October 13, 2021 19:19
@ricokahler ricokahler force-pushed the refactor/desk-tool-ts-coverage branch from fd77ed0 to 1863075 Compare October 13, 2021 20:00
@@ -45,7 +45,7 @@
"@sanity/uuid": "^3.0.1",
"hashlru": "^2.1.0",
"is-hotkey": "^0.1.6",
"leven": "^2.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -61,7 +61,7 @@
"inquirer": "^6.0.0",
"is-installed-globally": "^0.1.0",
"klaw-sync": "^4.0.0",
"leven": "^2.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -33,7 +33,7 @@
"@sanity/generate-help-url": "2.18.0",
"arrify": "^1.0.1",
"humanize-list": "^1.0.1",
"leven": "^2.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ricokahler ricokahler force-pushed the refactor/desk-tool-ts-coverage branch from 1863075 to 11c0e0e Compare October 13, 2021 21:16
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.

LGTM! 👏🏼

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.

None yet

3 participants