Skip to content

Conversation

@eliamartani
Copy link
Contributor

What kind of change does this PR introduce?

Refactors code as a part of hacktoberfest mentioned in #2621.
@Saeris @christianalfoni

What is the current behavior?

/app/src/app/pages/common/Modals/SelectSandboxModal/index.js was using inject from app/componentConnectors and written in JS.

What is the new behavior?

Uses useOvermind hook from app/overmind and was converted to tsx.

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

Checklist

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

@eliamartani eliamartani changed the title Refactor on /Modals/SelectSandboxModal/index.js 🔨 Refactor, 🧠 Overmind, Hacktoberfest - /app/pages/common/Modals/SelectSandboxModal/index.js Oct 16, 2019
@lbogdan lbogdan temporarily deployed to pr2816 October 16, 2019 15:51 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.

This is looking great! 😄 It did include a broken import, but I made a PR you can merge: eliamartani#1

Then we are ready, tested:

  • Open "Pick sandbox modal"
  • Select a different sandbox

@lbogdan lbogdan temporarily deployed to pr2816 October 17, 2019 12:47 Inactive
@eliamartani
Copy link
Contributor Author

This is looking great! 😄 It did include a broken import, but I made a PR you can merge: eliamartani#1

Then we are ready, tested:

  • Open "Pick sandbox modal"
  • Select a different sandbox

Hi @christianalfoni , thanks for your comment.

I accepted the pull request so you can review it again 😄

@Saeris Saeris added Hacktoberfest 🔨 Refactor 🧠 Overmind Indicates that this is related to the app's State Management labels Oct 21, 2019
@eliamartani
Copy link
Contributor Author

Hi guys, any chance of this PR being accepted?

Just let me know. Thanks.

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.

Great! :) Did same test again and it looks good!

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