Skip to content

Conversation

@ftonato
Copy link
Contributor

@ftonato ftonato commented Oct 15, 2019

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/pages/common/Modals/PRModal/index.js was using inject from app/componentConnectors and written in JS.

What is the new behavior?

uses useOvermind hook from app/overmind and converted to tsx.

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

Checklist

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

@lbogdan lbogdan temporarily deployed to pr2803 October 15, 2019 16:57 Inactive
@Saeris Saeris added Hacktoberfest 🔨 Refactor 🧠 Overmind Indicates that this is related to the app's State Management labels Oct 16, 2019
@lbogdan lbogdan temporarily deployed to pr2803 October 16, 2019 10:37 Inactive
@ftonato
Copy link
Contributor Author

ftonato commented Oct 18, 2019

@lbogdan should I solve the conflict or you'll fix it?

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.

Can you plese fix lint and update the imports?

@lbogdan lbogdan temporarily deployed to pr2803 October 22, 2019 09:10 Inactive
@lbogdan
Copy link
Contributor

lbogdan commented Oct 22, 2019

Build for latest commit b486fe0 is at https://pr2803.build.csb.dev/s/new.

@ftonato
Copy link
Contributor Author

ftonato commented Oct 23, 2019

@lbogdan is everything okay with this pr?

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! You just have to fix that linting error, you can just confirm the suggested change if you want to 😄

@christianalfoni
Copy link
Contributor

christianalfoni commented Oct 23, 2019

Added PR for you if you are unsure how to fix the import, it also includes the linting fix: ftonato#1

You can just merge that and we should be good 👍

@ftonato
Copy link
Contributor Author

ftonato commented Oct 28, 2019

Hello @christianalfoni, I have accepted your suggestion, can we merge now?

@CompuIves
Copy link
Member

Ready for merge! Thanks!

@CompuIves CompuIves merged commit 6764a2d into codesandbox:master Oct 28, 2019
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.

6 participants