Skip to content

Conversation

@MichaelDeBoey
Copy link
Contributor

Follow-up of #2743 & #2816

Things I did extra:

  • Change newSandboxShowcaseSelected's signature to accept a string instead of { id: string }, because it only has 1 argument
  • Put all styles into the elements.ts file
  • Make Sandbox a FunctionComponent
  • Move overmind subscriptions as close as possible to the components itself instead of passing it through
  • Make alias an optional argument of getSandboxName

@MichaelDeBoey MichaelDeBoey added 🔨 Refactor 🧠 Overmind Indicates that this is related to the app's State Management labels Nov 22, 2019
@lbogdan lbogdan temporarily deployed to pr3071 November 22, 2019 14:11 Inactive
@lbogdan
Copy link
Contributor

lbogdan commented Nov 22, 2019

Build for latest commit 6692d16 is at https://pr3071.build.csb.dev/s/new.

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.

It looks great, though there is a bug here (also in production). We need to close the modal as suggested. If you merge this in, it is ready 😄

Tested:

  • Open profile
  • Open select sandbox modal
  • Change sandbox
  • Refresh

(Suggested change must be merged first)

@MichaelDeBoey MichaelDeBoey force-pushed the overmind/SelectSandboxModal branch from d26878d to 5bbc834 Compare November 28, 2019 16:29
@lbogdan lbogdan temporarily deployed to pr3071 November 28, 2019 16:59 Inactive
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.

Amazeballs 👍 😄

Double checked that you can select a new sandbox, and modal also closes, as it should

@christianalfoni christianalfoni merged commit 40e4237 into codesandbox:master Dec 3, 2019
@MichaelDeBoey
Copy link
Contributor Author

Always a pleasure 🙂

@MichaelDeBoey MichaelDeBoey deleted the overmind/SelectSandboxModal branch December 3, 2019 11:40
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.

3 participants