Skip to content

Conversation

@MichaelDeBoey
Copy link
Contributor

Resubmission of #2843

Follow-up of #2782

Things I did extra:

  • Change sandboxPrivacyChanged 's signature to accept 0 | 1 | 2 instead of { privacy: 0 | 1 | 2 }, because it only has 1 argument
  • Put all styles into the elements.ts file
  • Move overmind subscriptions as close as possible to the components itself instead of passing it through
  • Extract Environment, ForkedFrom, Git, Privacy, PrivacyNotice & Team out of Project
  • Use FunctionComponent instead of React.FC, since it's the same, but FunctionComponent is a bit clearer I think

@MichaelDeBoey MichaelDeBoey added 🔨 Refactor 🧠 Overmind Indicates that this is related to the app's State Management labels Dec 3, 2019
@lbogdan
Copy link
Contributor

lbogdan commented Dec 3, 2019

Build for latest commit 008c3f9 is at https://pr3119.build.csb.dev/s/new.

@lbogdan lbogdan temporarily deployed to pr3119 December 4, 2019 13:31 Inactive
@MichaelDeBoey
Copy link
Contributor Author

Typecheck will pass once DefinitelyTyped/DefinitelyTyped#40894 is merged and @types/react-color is updated

@CompuIves
Copy link
Member

Interesting, why do we include @types/react-color?

@SaraVieira
Copy link
Contributor

@MichaelDeBoey Do you mind closing this and splitting it up? 42 files is a lot :(

@MichaelDeBoey
Copy link
Contributor Author

@CompuIves I included @types/react-color, to have a typed SketchPicker (used in TemplateConfig/index.tsx)

@SaraVieira I can make a new PR where I only have the first commit and a separate PR for the extraction of all the components?
Having each extraction a different PR will cause merge conflicts so

@christianalfoni
Copy link
Contributor

@MichaelDeBoey I think we can get this through as the refactor is scoped to a specific part of the page. We just have to test the Project feature as a whole 😄

Could you just add the type here as well? Just so that we can merge it in without having to wait for the other one?

@christianalfoni
Copy link
Contributor

Oh sorry, it is a different repo... can we get around this? Write a comment ignore the typing or something. Do not want to have this pending, as we do not know when the other one will be merged

Copy link
Contributor

@christianalfoni christianalfoni left a comment

Choose a reason for hiding this comment

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

Tested:

  • Open /s/new, it shows the project view correctly with views, forks, template bookmark, explorer, dependencies etc.
  • Forked it and opened SandboxInfo
  • Change name (alias)
  • Changed description
  • I see the profile, with the badges and can click to open the profile
  • Click "like", I see the views and forks as well
  • I can add and remove tags (tested with refresh)
  • I can change the privacy setting (tested with refresh)
  • Can toggle "frozen" (tested with refresh)
  • Template name is there and I can click it
  • Environment is there and correct
  • Create template works
  • I can edit color and icon (tested with refresh)
  • Delete template works
  • Delete Sandbox works
  • Shows team indication when a team Sandbox

@SaraVieira
Copy link
Contributor

@MichaelDeBoey Can you get the react color types off and then when thats merged we can add it?

Wanna get a bunch of these things in this month :)

@SaraVieira SaraVieira merged commit 008c3f9 into codesandbox:master Dec 10, 2019
@SaraVieira
Copy link
Contributor

I deleted the @types/react-color and merged it :)

Good luck with thier pr 🎉

@MichaelDeBoey MichaelDeBoey deleted the overmind/Project branch December 10, 2019 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🧠 Overmind Indicates that this is related to the app's State Management 🔨 Refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants