Skip to content
This repository has been archived by the owner on May 13, 2024. It is now read-only.

✨ Inline theme selection at on-boarding #512

Merged
merged 1 commit into from
Jul 23, 2019
Merged

Conversation

imptrx
Copy link
Contributor

@imptrx imptrx commented Jul 9, 2019

Spec: brave/brave-browser#4992
Quick View:
welcome-theme

Changes

  • inline option to select theme during onboarding

Test plan

  • go to brave://welcome
  • at the 4th screen during the on-boarding process you now have a dropdown to select theme
  • button is disabled unless a valid option is selected
  • clicking the confirm should go to next screen
Link / storybook path to visual changes

Integration

  • Does this contain changes to src/components or src/

    • Will you publish to npm immediately after this PR, or wait until sometime in the future?
    • Incompatible API change to something existing (major version increase)
    • Adding new backwards-compatible functionality? (minor version increase)
    • Fixing a bug backwards-compatibly? (patch version increase)
  • Does this contain changes to src/features for brave-core?

    • Are there non backwards-compatible changes required for brave-core? Do not merge until brave-core PR is approvable. Link to brave-core PR:
    • Will you create brave-core PR to update to this commit after it is merged?
    • Wants uplift to brave-core feature branch?
      • When uplift-approved, merge to brave-core-0.VV.x feature branch
      • Create additional brave-core PRs for each feature branch to update commit

Copy link
Contributor

@rossmoody rossmoody left a comment

Choose a reason for hiding this comment

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

Looks great. Curious how this will tie into this one? #487

@imptrx
Copy link
Contributor Author

imptrx commented Jul 10, 2019

Will try to get this and #487 in at the same time to maximize the difference when the user changes the settings 😃

@petemill
Copy link
Member

petemill commented Jul 10, 2019

One concern I have with this UX @rossmoody is that the user may feel that they have to pick a theme, and not know that 'Same as {macOS,Windows}' is the default. Therefore they may explicitly choose Light / Dark, and not know that they could have it follow the OS when they changed it.

To be honest, I think having this choice on the welcome screen had high value, before we matched system theme, especially if it wasn't a drop-down but was a visual choice, but now it seems confusing perhaps.

I don't know if there's a way we can offer this tri-state in a way that doesn't prompt users to override the default unnecessarily?

image

Edit: Perhaps the choices in this welcome screen could be different to what's offered in the Settings page, in that we offer 2 choices: Dark or Light (ideally graphically). The choice is pre-selected to whatever the system theme is (or Light if not on an OS which supports dark mode). Only if the user changes to a theme that the OS is not currently on, do we save a setting as specificall 'Light' or 'Dark'. Otherwise, we leave it at 'System'. WDYT @rossmoody ?

@petemill
Copy link
Member

Also, how about we don't have a Confirm button, but have the change previewed and set immediately?

@rossmoody
Copy link
Contributor

rossmoody commented Jul 11, 2019

The original goal of this screen was just to show users that they could take advantage of dark mode cause it wasn't offered in Muon. Since we're defaulting to following system setting, the delight and surprise of this screen will more likely be discovered in screen number one and I don't think (though I wish we could test) that many people opt to use light them in dark system settings. I do think (for the moment) this screen is worth keeping though because Chrome doesn't allow light system theme and dark browser theme and we do. (That said I also think this could get the boot in the next rev of the welcome flow for something stronger).

Maybe this could be solved immediately with a default designation in the list item: System theme (default)

I much prefer your idea though. Maybe there are the three options but System theme (default) is preselected when the user arrives. Maybe we do the updates as a followup?

@rossmoody
Copy link
Contributor

I started experimenting with the new approach here but it feels like it's own initiative that needs review and probably shouldn't block this PR.

image

I'd suggest we move forward with this for now and add a (default) to the System list item so it will read:

System theme (default) @imptrx @rebron

@rebron
Copy link
Collaborator

rebron commented Jul 12, 2019

@rossmoody Sounds good. Let's go with System theme (default)

@petemill
Copy link
Member

Should we keep or remove the Confirm button? Surely without it we won't see the browser theme change from light to dark in realtime?

@rossmoody
Copy link
Contributor

Keep for now, it's really just to give the user a place to feel like they solidified the decision though I'm fairly certain the dropdown will behave like it does in settings. If Dark theme is selected in the dropdown the theme will change to dark and then "Confirming" it will advance to the next screen.

@petemill
Copy link
Member

Keep for now, it's really just to give the user a place to feel like they solidified the decision though I'm fairly certain the dropdown will behave like it does in settings. If Dark theme is selected in the dropdown the theme will change to dark and then "Confirming" it will advance to the next screen.

So just to confirm that "Confirm" won't actually save the setting

@rossmoody
Copy link
Contributor

Right, under the hood it's set when the selection is made.

@imptrx imptrx requested a review from rossmoody July 23, 2019 23:03
@imptrx imptrx merged commit ecceb0f into master Jul 23, 2019
@imptrx imptrx deleted the welcome-set-theme branch July 23, 2019 23:22
@imptrx
Copy link
Contributor Author

imptrx commented Jul 24, 2019

These changes will need to be pulled into brave/brave-core#2909 since the migration of welcome specific ui: brave/brave-core#2896

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants