Skip to content

Conversation

@br1anchen
Copy link
Contributor

What kind of change does this PR introduce?

Refactor for #2621 @Saeris @christianalfoni

What is the current behavior?

They are in js with cerebral

What is the new behavior?

They are changed to ts files with Overmind

What steps did you take to test this? This is required before we can merge, make sure to test the flow you've updated.

  1. yarn test
  2. yarn lint
  3. yarn typecheck
  4. yarn start, go to Dashboard, drag things no error

Checklist

  • Documentation
  • Testing
  • Ready to be merged
  • Added myself to contributors table

@lbogdan
Copy link
Contributor

lbogdan commented Oct 19, 2019

Build for latest commit 560923f is at https://pr2863.build.csb.dev/s/new.

@br1anchen br1anchen changed the title Brian/chore/refactor selected sandbox items 🔨 Refactor, 🧠 Overmind | packages/app/src/app/pages/Dashboard/Content/DragLayer/SelectedSandboxItems Oct 19, 2019
@br1anchen br1anchen changed the title 🔨 Refactor, 🧠 Overmind | packages/app/src/app/pages/Dashboard/Content/DragLayer/SelectedSandboxItems 🔨 Refactor, 🧠 Overmind, Hactoberfest | packages/app/src/app/pages/Dashboard/Content/DragLayer/SelectedSandboxItems Oct 19, 2019
Comment on lines -40 to -41
left={left}
top={top}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you not passing this one down anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I removed these props is because they are not used any where by any components under <SelectedSandboxItems />.
You can see they are forwarded in here to <AnimatedSandboxItem />, but <AnimatedSandboxItem /> does not use them and not expects them either here.
It might be a bug for <AnimatedSandboxItem /> not using them, then we can fix it with more context knowledge.

Copy link
Contributor

@SaraVieira SaraVieira left a comment

Choose a reason for hiding this comment

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

I don't know if it does need changes but its to keep track of the PR's better

@br1anchen
Copy link
Contributor Author

@SaraVieira Thanks for review. No problem to request changes. It is a good way to track it 👍

@Saeris Saeris added Hacktoberfest 🔨 Refactor 🧠 Overmind Indicates that this is related to the app's State Management labels Oct 21, 2019
@br1anchen br1anchen requested a review from SaraVieira October 22, 2019 09:32
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.

This looks great! 😄 👍

Tested:

  • Dragging single sandbox
  • Dragging multiples sandboxes
  • Drop sandbox in trash

@christianalfoni christianalfoni merged commit a7b3784 into codesandbox:master Oct 23, 2019
@br1anchen br1anchen deleted the brian/chore/refactor-SelectedSandboxItems branch October 23, 2019 12:08
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