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

Implements latest Settings spec #1423

Merged
merged 4 commits into from Jul 24, 2019
Merged

Implements latest Settings spec #1423

merged 4 commits into from Jul 24, 2019

Conversation

keianhzo
Copy link
Collaborator

@keianhzo keianhzo commented Jul 18, 2019

Fixes #1372 #962 #1440 This PR implements the latests Settings spec which also includes the new user agent switch button in the navigation bar.

@philip-lamb
Copy link
Contributor

Awesome. Will give it a test tomorrow.

@keianhzo keianhzo force-pushed the settings_updates branch 2 times, most recently from fc993e3 to 9ec17a9 Compare July 18, 2019 15:14
Copy link
Contributor

@MortimerGoro MortimerGoro left a comment

Choose a reason for hiding this comment

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

  • I have tried to switch between desktop and VR UAs. It works on the current page, but when browsing to other pages I got a desktop viewport while the VR icon was visible in the URL bar

  • I think the VR text icon can be confusing for users. Can't we use a similar icon to the one used in Oculus browser? Toggle between the desktop icon with or without the checkmark.

  • The VR icon is too close to the URL bar end on the content feed and looks misaligned. Should we hide it on the content feed? (in that case the bookmark icon is not visible)

@MortimerGoro
Copy link
Contributor

MortimerGoro commented Jul 19, 2019

cc @jvonitter @thenadj related to the checkmark idea I mentioned:

Firefox desktop uses this icon to enable Mobile/Responsive layout. It changes color when enabled/disabled.
Captura de pantalla 2019-07-19 a las 11 53 18

Firefox for iOS uses this icon to go back to Mobile/Responsive layout once desktop is enabled. It shows/hides the blue check when enabled/disabled
Captura de pantalla 2019-07-19 a las 11 58 51

@bluemarvin
Copy link
Contributor

I'm not sure if it is related but the first time I ran this PR only the main window was visible, no other UI was visible. I had to restart to see the rest of the UI like navigation bar and tray.

@bluemarvin
Copy link
Contributor

Also, I really do not like the VR in the navigation bar. It looks very heavy and it's purpose is not immediately obvious.

@jvonitter
Copy link
Contributor

jvonitter commented Jul 23, 2019

@keianhzo new icons!
UA_Icons.zip

@MortimerGoro MortimerGoro merged commit 795ba86 into master Jul 24, 2019
@bluemarvin bluemarvin deleted the settings_updates branch July 26, 2019 22:37
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.

Settings IA and verbiage updates
5 participants