-
Notifications
You must be signed in to change notification settings - Fork 2.4k
🔨 Refactor, 🧠 Overmind Hactoberfest | Refactor /app/pages/common/Modals/index.js #2902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🔨 Refactor, 🧠 Overmind Hactoberfest | Refactor /app/pages/common/Modals/index.js #2902
Conversation
|
Build for latest commit bed89b4 is at https://pr2902.build.csb.dev/s/new. |
| }; | ||
|
|
||
| export const modalClosed: Action = ({ state, effects }) => { | ||
| export const modalClosed: Action<any> = ({ state, effects }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SaraVieira not sure if it's the right way
…lient into use-overmind-modals
| return ( | ||
| <ThemeProvider | ||
| theme={{ | ||
| templateColor: templateColor(sandbox, templateDef), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SaraVieira also need your inputs here as types for templateColor's second argument and templateDef are incompatible.
christianalfoni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent stuff! About the typings it needs some review no matter, so this is perfectly okay for now 😄
Tested:
- Opening/closing several modals
What kind of change does this PR introduce?
Refactors code as a part of hacktoberfest mentioned in 2621 @SaraVieira, @Saeris
What is the current behavior?
/app/pages/common/Modals/index.js using Cerebral
What is the new behavior?
/app/pages/common/Modals/index.js using 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.
Checklist