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

Mobile Preferences Screen Prototype #1472

Merged

Conversation

ghalestrilo
Copy link
Collaborator

@ghalestrilo ghalestrilo commented Jun 23, 2020

Part of the Mobile UI project. This PR implements a screen to access and control editor settings.

This PR requires #1467 to be merged

How to test:

  1. Run the app locally with the MOBILE_ENABLED flag set to true (MOBILE_ENABLED=true npm start)
  • You should be able to access /mobile
  • You should be able to edit code
  • Clicking the gear icon should take you to the settings screen
  • You should be able to change settings and go back to the editor
  • Preview Screen should be working as well
  1. Run the app without the flag enabled
  • It should work as usual
  • /mobile routes should not be accessible

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

@ghalestrilo ghalestrilo requested a review from andrewn June 24, 2020 19:23
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.

Screen Shot 2020-06-29 at 5 31 10 PM

I'm also noticing some styles on the Preferences not being right, specifically the "X" next to "Preferences" (should be in the right corner)? and I also notice that "Used with screen reader" is not the right color for the theme.

const textColor = prop('primaryTextColor');

const IconButton = styled.button`
width: 3rem;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to extend from client/common/Button? No worries if that's overcomplicated. Either way, I'd like this to use remSize so the measurement can easily be read as pixels.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I updated it

@@ -0,0 +1,57 @@
import React from 'react';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this file could have a more descriptive/specific name since "Selector" feels pretty generic, where this component is specifically being used for a Preference selection.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I renamed it to <PreferencePicker /> to be more descriptive

client/modules/IDE/components/PreviewFrame.jsx Outdated Show resolved Hide resolved
client/modules/IDE/pages/MobileIDEView.jsx Outdated Show resolved Hide resolved
@catarak
Copy link
Member

catarak commented Jun 30, 2020

When trying this on my phone, the "On" and "Off" text felt too small. In the light theme, the contrast between "On" being selected or "Off" being selected feels too subtle, though maybe that needs to be addressed in a separate PR.

A design idea would be to make the "Settings X" at the top into a bar, with a separate background color, maybe the same color as on the main page (the panel color).

@ghalestrilo
Copy link
Collaborator Author

I increased the option label font-size to 16px on the mobile view, that should be a bit better
About the Header, I think we need to test it on the second iteration. I thought leaving it transparent conveyed the idea that it's a "temporary" Screen / Modal, with a different purpose from the ones with solid headers.

@catarak
Copy link
Member

catarak commented Jul 1, 2020

About the Header, I think we need to test it on the second iteration. I thought leaving it transparent conveyed the idea that it's a "temporary" Screen / Modal, with a different purpose from the ones with solid headers.

Sounds good! I think it's worth testing, as when I opened the screen, the hierarchy between "Settings" and "General Settings" felt confusing to me as they are both the same font size and styling.

@catarak
Copy link
Member

catarak commented Jul 1, 2020

@ghalestrilo looks great! I just pushed a commit that re-added the Redux DevTools to App.jsx... maybe this could be done with an environment variable so that you don't have to comment/uncomment it all of the time?

@catarak catarak merged commit 69bd6cb into processing:develop Jul 1, 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