Skip to content
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

Merged

Conversation

ghalestrilo
Copy link
Collaborator

Requires #1502 to be merged

This branch refactors <Console />, <MobileIDEView /> and <MobileSketchView /> to use redux hooks, rather than connect

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • is from a uniquely-named feature branch and has been rebased on top of the latest master. (If I was asked to make more changes, I have made sure to rebase onto master then too)
  • is descriptively named and links to an issue number, i.e. Fixes #123

Copy link
Member

@catarak catarak left a 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') => [
Copy link
Member

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.

Copy link
Collaborator Author

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

@ghalestrilo ghalestrilo requested a review from catarak July 21, 2020 21:19
Copy link
Member

@catarak catarak left a 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 😸

@ghalestrilo
Copy link
Collaborator Author

Awesome! Let's do it 💪

@ghalestrilo ghalestrilo merged commit 7dbcde4 into processing:develop Jul 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants