-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Refactor mobile components to react hooks #1507
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 mobile components to react hooks #1507
Conversation
…factor-mobile-hooks
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.
Overall, this looks great to me! One small UX thing I would change (besides the bigger issue of figuring out how to display the console with the sketch preview, which can definitely be saved for another PR), is that I expected the console to toggle when I tapped the console icon, i.e. if it were open I expected tapping it to close it. But also, maybe that's best left for another PR 😄
@@ -0,0 +1,26 @@ | |||
export const optionsOnOff = (name, onLabel = 'On', offLabel = 'Off') => [ |
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.
Since Preferences
is not in client/common
, I think it makes sense to put this file next to the Preferences
component. You could even put them in a subfolder together called Preferences
, and the component could be index.jsx
and this could be a helper file in that folder.
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.
Great idea, will do that
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.
Thanks for making those changes! I think this is ready to merge 😸
Awesome! Let's do it 💪 |
Requires #1502 to be merged
This branch refactors
<Console />
,<MobileIDEView />
and<MobileSketchView />
to use redux hooks, rather thanconnect
I have verified that this pull request:
npm run lint
)Fixes #123