Skip to content

Conversation

@MichaelDeBoey
Copy link
Contributor

@MichaelDeBoey MichaelDeBoey commented Dec 3, 2019

Resubmission of #2848

Follow-up of #2623

Things I did extra:

  • Move overmind subscriptions as close as possible to the components itself instead of passing it through
  • Use a ref for TokenInput's onClick instead of using the MouseEvent's target
  • Export CLI by a named export instead of a default export

@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 4, 2019

Build for latest commit 4d52717 is at https://pr3121.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.

Looks great! 👍 😄

Tested:

  • Opening cli/login
  • When logged in it showed the code
  • Logged out, now shows login button
  • Logged in, shows the code
  • Click the code and it selects the whole code

@lbogdan lbogdan temporarily deployed to pr3121 December 4, 2019 11:15 Inactive
}

if (loading) {
if (isLoadingCLI) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did this come from?

Was it changed in the state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was first passed to Prompt as a prop by CLI, but can just be taken from the useOvermind hook

Copy link
Contributor

Choose a reason for hiding this comment

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

OHH

Cool! Will merge :)

@SaraVieira SaraVieira merged commit 108a567 into codesandbox:master Dec 8, 2019
@MichaelDeBoey MichaelDeBoey deleted the overmind/CLI branch December 8, 2019 18:19
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.

4 participants