Skip to content

Conversation

@ganes1410
Copy link
Contributor

What kind of change does this PR introduce?

Refactor #2621 @Saeris @christianalfoni

What is the current behavior?

It was using class component with js file, and cerebral for state management

What is the new behavior?

It now uses tsx instead of js. And Overmind is used for state management with help of useOvermind hook

What steps did you take to test this? This is required before we can merge.

  1. yarn lint
  2. yarn test
  3. yarn typecheck

Checklist

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

In this PR,I changed another file, because it was throwing a typescript error.

@vercel vercel bot temporarily deployed to staging October 13, 2019 17:23 Inactive
@vercel
Copy link

vercel bot commented Oct 13, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/codesandbox/codesandbox-client/9o6q8i9q7
🌍 Preview: https://codesandbox-cl-git-fork-ganes1410-refactor-sandbox-profi-9c6225.codesandbox1.now.sh

@ganes1410 ganes1410 changed the title Refactor /app/pages/Profile/Sandboxes/index.js to `/app/pages/Profi… Refactor /app/pages/Profile/Sandboxes/index.js to /app/pages/Profile/Sandboxes/index.tsx 🔨 Refactor 🧠 Overmind Hacktoberfest Oct 13, 2019
@Saeris Saeris added Hacktoberfest 🔨 Refactor 🧠 Overmind Indicates that this is related to the app's State Management labels Oct 14, 2019
Copy link
Contributor

@Saeris Saeris left a comment

Choose a reason for hiding this comment

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

Requested some minor changes, you can just batch and commit the suggestions. Otherwise looks great! We don't have a test suite for this specific part of the app, so can you also please manually verify that this still works? I'll try to take a look myself later this week.

@ganes1410
Copy link
Contributor Author

@Saeris I have added the required change. Let me know if you need me to make any more changes.

@lbogdan lbogdan temporarily deployed to pr2765 October 21, 2019 17:01 Inactive
@SaraVieira
Copy link
Contributor

Hey!

I am so sorry it took so long to review this from me but unfortatly this file is now being redesigned and will be merged soon so there is no need to merge this :(

I am so sorry and I hope you understand
You can see the new PR by Drake here: #3115

@SaraVieira SaraVieira closed this Dec 10, 2019
@ganes1410 ganes1410 deleted the refactor-sandbox-profile-index branch December 11, 2019 05: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