Select default browser on welcome page import page #17270

spylogsster · 2023-02-17T12:24:23Z

Google Chrome Dev/Google Chrome Beta on other then EN locales have translated name, so we need to read Google Chrome translations, moved it to separate issue brave/brave-browser#28717

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

step from issue

brave-builds · 2023-02-17T13:41:04Z

A Storybook has been deployed to preview UI for the latest push

nullhook

Can you please paste a screenshot?

spylogsster · 2023-02-17T17:44:27Z

Can you please paste a screenshot?

brave-builds · 2023-02-17T18:29:10Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-02-22T12:57:24Z

A Storybook has been deployed to preview UI for the latest push

components/brave_welcome_ui/components/select-browser/index.tsx

nullhook

Please make sure if you've tested the code with the React.useEffect change

components/brave_welcome_ui/components/select-browser/index.tsx

spylogsster · 2023-02-22T16:45:00Z

Please make sure if you've tested the code with the React.useEffect change

it works

nullhook

lgtm

brave-builds · 2023-02-22T17:31:53Z

A Storybook has been deployed to preview UI for the latest push

simonhong

++ with some suggestion! 👍🏼

browser/ui/webui/brave_welcome_ui.cc

simonhong · 2023-02-23T02:03:38Z

I tested with Chrome, Edge and Whale browser and only works when Edge was default. Can you check with other browsers?

brave-builds · 2023-02-23T14:38:36Z

A Storybook has been deployed to preview UI for the latest push

spylogsster · 2023-02-23T14:57:19Z

Google Chrome Dev/Google Chrome Beta on other then EN locales have translated name, so we need to read Google Chrome translations, after discussion with @mkarolin decided to move it to separate issue brave/brave-browser#28717

chromium_src/chrome/browser/importer/importer_list.cc

mkarolin

chromium_src changes LGTM

bsclifton · 2023-02-23T17:02:13Z

Changes LGTM (I pulled down code, built, and ran). There was some behavior I noticed which was interesting:

Clicking set as default browser opens the defaults app on Windows. Not sure if that's expected - I know you didn't change it, but I tested because I was curious if default is changed on screen 1, what happens on screen 2. macOS and Linux might have an issue if it's able to set because then (on screen 2, import) the default is no longer Chrome, Firefox, etc. It's now Brave
UPDATE: tracked in Onboarding: setting as default may break import flow brave-browser#28787
There's a Chromium-based browser which shows up but I can't pick it. I think this is Brave. This also isn't related to changes here, just noting. If it's not selectable, maybe we should remove it
UPDATE: may be tracked in Do not show/Hide Chromium based-browser card for any undetected/not supported browsers in the brave://welcome screen brave-browser#28211
If nothing is selected, the Import button is not disabled. It doesn't do anything - but it's clickable
UPDATE tracked in Onboarding: Import button should be disabled if there's no selection brave-browser#28788

bsclifton

Found a few things not related to this PR - we may want to make separate issues for them (cc: @rebron)

But changes here LGTM

spylogsster requested review from bsclifton, simonhong and nullhook February 17, 2023 12:24

github-actions bot assigned spylogsster Feb 17, 2023

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Feb 17, 2023

spylogsster force-pushed the brave-28573 branch from e3da494 to d0e3116 Compare February 17, 2023 12:32

nullhook requested changes Feb 17, 2023

View reviewed changes

spylogsster force-pushed the brave-28573 branch from d0e3116 to 0dcce26 Compare February 17, 2023 17:29

spylogsster force-pushed the brave-28573 branch from 0dcce26 to 59de9ac Compare February 22, 2023 11:46

nullhook requested changes Feb 22, 2023

View reviewed changes

components/brave_welcome_ui/components/select-browser/index.tsx Outdated Show resolved Hide resolved

spylogsster force-pushed the brave-28573 branch from 59de9ac to 61ad98f Compare February 22, 2023 15:51

nullhook requested changes Feb 22, 2023

View reviewed changes

components/brave_welcome_ui/components/select-browser/index.tsx Outdated Show resolved Hide resolved

spylogsster force-pushed the brave-28573 branch from 61ad98f to 8ccea16 Compare February 22, 2023 16:44

spylogsster force-pushed the brave-28573 branch from 8ccea16 to 62cbc16 Compare February 22, 2023 16:49

nullhook approved these changes Feb 22, 2023

View reviewed changes

simonhong approved these changes Feb 23, 2023

View reviewed changes

browser/ui/webui/brave_welcome_ui.cc Outdated Show resolved Hide resolved

browser/ui/webui/brave_welcome_ui.cc Outdated Show resolved Hide resolved

browser/ui/webui/brave_welcome_ui.cc Outdated Show resolved Hide resolved

spylogsster marked this pull request as draft February 23, 2023 04:59

spylogsster force-pushed the brave-28573 branch 4 times, most recently from d0ddf2f to 78d49e9 Compare February 23, 2023 06:42

Select default browser on welcome page import page

ccd5641

spylogsster force-pushed the brave-28573 branch from 78d49e9 to 91539b6 Compare February 23, 2023 07:00

spylogsster added the CI/run-draft label Feb 23, 2023

spylogsster force-pushed the brave-28573 branch from 91539b6 to 6710b9c Compare February 23, 2023 13:44

Sync browser type names

f80f5ff

spylogsster force-pushed the brave-28573 branch from 6710b9c to f80f5ff Compare February 23, 2023 13:52

spylogsster marked this pull request as ready for review February 23, 2023 14:47

spylogsster requested a review from a team as a code owner February 23, 2023 14:47

mkarolin reviewed Feb 23, 2023

View reviewed changes

chromium_src/chrome/browser/importer/importer_list.cc Show resolved Hide resolved

mkarolin approved these changes Feb 23, 2023

View reviewed changes

bsclifton approved these changes Feb 23, 2023

View reviewed changes

spylogsster merged commit 845aa85 into master Feb 24, 2023

spylogsster deleted the brave-28573 branch February 24, 2023 02:24

github-actions bot added this to the 1.50.x - Nightly milestone Feb 24, 2023

bsclifton mentioned this pull request Feb 27, 2023

Onboarding: setting as default may break import flow brave/brave-browser#28787

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Select default browser on welcome page import page #17270

Select default browser on welcome page import page #17270

spylogsster commented Feb 17, 2023 •

edited

Loading

brave-builds commented Feb 17, 2023

nullhook left a comment

spylogsster commented Feb 17, 2023 •

edited

Loading

brave-builds commented Feb 17, 2023

brave-builds commented Feb 22, 2023

nullhook left a comment

spylogsster commented Feb 22, 2023

nullhook left a comment

brave-builds commented Feb 22, 2023

simonhong left a comment

simonhong commented Feb 23, 2023

brave-builds commented Feb 23, 2023

spylogsster commented Feb 23, 2023

mkarolin left a comment

bsclifton commented Feb 23, 2023 •

edited

Loading

bsclifton left a comment

Select default browser on welcome page import page #17270

Select default browser on welcome page import page #17270

Conversation

spylogsster commented Feb 17, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Feb 17, 2023

nullhook left a comment

Choose a reason for hiding this comment

spylogsster commented Feb 17, 2023 • edited Loading

brave-builds commented Feb 17, 2023

brave-builds commented Feb 22, 2023

nullhook left a comment

Choose a reason for hiding this comment

spylogsster commented Feb 22, 2023

nullhook left a comment

Choose a reason for hiding this comment

brave-builds commented Feb 22, 2023

simonhong left a comment

Choose a reason for hiding this comment

simonhong commented Feb 23, 2023

brave-builds commented Feb 23, 2023

spylogsster commented Feb 23, 2023

mkarolin left a comment

Choose a reason for hiding this comment

bsclifton commented Feb 23, 2023 • edited Loading

bsclifton left a comment

Choose a reason for hiding this comment

spylogsster commented Feb 17, 2023 •

edited

Loading

spylogsster commented Feb 17, 2023 •

edited

Loading

bsclifton commented Feb 23, 2023 •

edited

Loading