-
Notifications
You must be signed in to change notification settings - Fork 2.4k
🔨 Refactor, 🧠 Overmind, refactored OpenedTabs to use Overmind #2769
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, 🧠 Overmind, refactored OpenedTabs to use Overmind #2769
Conversation
|
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/codesandbox/codesandbox-client/82cm0vdh4 |
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.
There's a few type errors that need to be resolved but otherwise this looks good! Please submit the requested changes and fix the following errors and I can merge this.
src/app/pages/Sandbox/Editor/Workspace/OpenedTabs/index.tsx(11,8): error TS1192: Module '"/home/circleci/codesandbox-client/packages/app/src/app/pages/Sandbox/Editor/Workspace/WorkspaceItem/index"' has no default export.
src/app/pages/Sandbox/Editor/Workspace/OpenedTabs/index.tsx(26,30): error TS2339: Property 'moduleShortid' does not exist on type 'ModuleTab | DiffTab'.
Property 'moduleShortid' does not exist on type 'DiffTab'.
src/app/pages/Sandbox/Editor/Workspace/OpenedTabs/index.tsx(72,42): error TS2345: Argument of type '{ tabIndex: number; }' is not assignable to parameter of type 'number'.Thanks!
packages/app/src/app/pages/Sandbox/Editor/Workspace/OpenedTabs/index.tsx
Outdated
Show resolved
Hide resolved
packages/app/src/app/pages/Sandbox/Editor/Workspace/OpenedTabs/index.tsx
Outdated
Show resolved
Hide resolved
|
Thanks! I'll revisit this soon and adjust my PR |
562b096 to
85403ce
Compare
|
Build for latest commit 8c0d486 is at https://pr2769.build.csb.dev/s/new. |
CompuIves
left a comment
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.
Heyo! Do you have time for the changes? Then it's ready to merge.
|
Hi @CompuIves - Yeah I was going to attempt to make the changes @Saeris requested, probably in the next day or two. Typescript is pretty new to me so theres a little bit of a learning curve for me. |
I actually probably will not get to this until November 9th. If it can wait I can work on it then - otherwise please take this branch forward in the meantime. |
|
Hi @fullstackzach! Did you get the time to check this one out yet? 🤔 |
|
@fullstackzach I created a PR you can just approve on your end :) fullstackzach#1 It got quite complicated. Just merge it in and we will do a test on our end as well to verify before merging in. |
|
Hey all - sorry for the delay on this, life got quite busy. I’ll take a look at it tonight |
50e942f to
4a9fef6
Compare
|
I made changes based on @Saeris's feedback to use a named export, but now when I run and running |
christianalfoni
left a comment
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.
Tested:
- Checked styling and icons of explorer, looking good!
OpenedTabs is a component not in use actually, so all okay. Nice to get the typing fixed!
What kind of change does this PR introduce?
This PR Refactors
/app/pages/Sandbox/Editor/Workspace/OpenedTabs/index.jsto use Overmind#2621
What steps did you take to test this? This is required before we can merge.
yarn testsuccessfullyyarn lintsuccessfullyyarn startand confirmed that Tabs functionality in the workspace still worksyarn typechecksuccessfullyChecklist