Skip to content

Conversation

@MichaelDeBoey
Copy link
Contributor

Follow-up of #2663

Things I did extra:

  • Change createLiveClicked's signature to accept a string instead of { sandboxId: string }, because it only has 1 argument
  • Change onChatEnabledChange's signature to accept a boolean instead of { enabled: boolean }, because it only has 1 argument
  • Remove unnecessary use of a Fragment (<></>)

@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 pr3063 November 22, 2019 12:37 Inactive
@lbogdan
Copy link
Contributor

lbogdan commented Nov 22, 2019

Build for latest commit a8c21c2 is at https://pr3063.build.csb.dev/s/new.

export const createLiveClicked: AsyncAction<{
sandboxId: string;
}> = async ({ state, effects, actions }, { sandboxId }) => {
export const createLiveClicked: AsyncAction<string> = async (
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, wouldn't this be less explicit (eg. I now need to know what's passed by going to the function call) and scalable (what if we want to add another option)? What's your opinion on this @christianalfoni?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this in recommendation of @christianalfoni in an earlier PR, but can't find it anywhere tho 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I remember this was christian idea in new code as it makes it cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah @christianalfoni said we should defo do it in new code.
But if I wanted, I could refactor them on the go

Can't find the comment where he said that anymore tho.
Too many PRs 😂😇

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yeah, there is no reason to pass an object anymore to actions. It was a requirement in Cerebral.

You could argue that the typing does not explicitly say sandboxId anymore, but payload. I just favour reduced syntax as if you were to write this action from scratch you would type it with string, because of our lazyness as developers 😂

@lbogdan lbogdan temporarily deployed to pr3063 November 30, 2019 23:55 Inactive
@lbogdan lbogdan temporarily deployed to pr3063 December 3, 2019 19:59 Inactive
@MichaelDeBoey MichaelDeBoey force-pushed the overmind/Live branch 2 times, most recently from 75bd79c to 422a361 Compare December 4, 2019 17:52
@lbogdan lbogdan temporarily deployed to pr3063 December 4, 2019 18:37 Inactive
@SaraVieira
Copy link
Contributor

@CompuIves Can you please review this?

Copy link
Member

@CompuIves CompuIves left a comment

Choose a reason for hiding this comment

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

That makes sense, this looks good to me! Let's merge it if you've tested the component 😄.

Thanks for this conversion!

@SaraVieira
Copy link
Contributor

YAY! All looks good to me too! Thank you!

@SaraVieira SaraVieira merged commit 75d4692 into codesandbox:master Dec 10, 2019
@MichaelDeBoey
Copy link
Contributor Author

With pleasure! 🙂

@MichaelDeBoey MichaelDeBoey deleted the overmind/Live branch December 10, 2019 11:44
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