-
Notifications
You must be signed in to change notification settings - Fork 920
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
New Onboarding WebUI #15637
New Onboarding WebUI #15637
Conversation
472e696
to
583c8b7
Compare
abef180
to
70da9fe
Compare
aec2dd9
to
a5a8f75
Compare
@nullhook Here are the assets, if theyre not as you need them, lets discuss. For the Browser icons, i can set up them all up and export them, but we will need a list of browsers we support. Or we can see if there is a someway we can get them online so we don't have to update them. |
8bea808
to
8b1b778
Compare
A Storybook has been deployed to preview UI for the latest push |
} | ||
}); | ||
|
||
+ currentAnimation.addEventListener("complete", setCompleteEvent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to change this whole patch to a 1-liner then perhaps you can try:
currentAnimation.addEventListener("complete", () => postMessage({ name: 'COMPLETE' });
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
patch updated
36d7308
to
0ef9fcb
Compare
A Storybook has been deployed to preview UI for the latest push |
01eec7c
to
96550b6
Compare
96550b6
to
d20570a
Compare
A Storybook has been deployed to preview UI for the latest push |
d20570a
to
51c11bf
Compare
A Storybook has been deployed to preview UI for the latest push |
61b9e35
to
a73bbdd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chromium_src
++ (see a couple other comments).
|
A Storybook has been deployed to preview UI for the latest push |
a73bbdd
to
6eeae10
Compare
A Storybook has been deployed to preview UI for the latest push |
Fixed the SonarCloud error. It was complaining about |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to go for me once this minor feedback is resolved, @mkarolin 's too. I'm approving since I'm on PTO and not as accessible, but please address non-nit feedback.
RecordP3AHistogram(screen_number_, finished_); | ||
} | ||
|
||
void WelcomeDOMHandler::HandleOpenSettingsPage(const base::Value::List& args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I don't think you need to do this - can just have <a href="chrome://settings/privacy" target="_blank">
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried adding an anchor and it doesn't seem to allow internal links to open. I believe there's an allow list somewhere?
let results = profiles | ||
.filter((profile) => profile.name !== 'Bookmarks HTML File') | ||
.map((profile) => { | ||
const browserType = getBrowserName(profile.name) | ||
// Introducing a new property here | ||
return { ...profile, browserType } | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React.useMemo
this calculation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why memoize it? It just get called once, on mount.
const [shouldPlayAnimations] = React.useState( | ||
loadTimeData.getBoolean('hardwareAccelerationEnabledAtStartup') && | ||
!window.matchMedia('(prefers-reduced-motion: reduce)').matches | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this doesn't change, seems like this doesn't need to be state, but could use memo
const shouldPlayAnimations = React.useMemo(() =>
loadTimeData.getBoolean('hardwareAccelerationEnabledAtStartup') &&
!window.matchMedia('(prefers-reduced-motion: reduce)').matches
, [])
At least saves calling matchMedia on every render.
6eeae10
to
c266e5e
Compare
A Storybook has been deployed to preview UI for the latest push |
Allow WebSerial API to be enabled via brave://flags.
Resolves brave/brave-browser#26378
DO NOT MERGE BEFORE: #15674The PR adds the new onboarding UI. It replaces the previous
brave://welcome
UI.Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
Visit
brave://welcome
and go through the steps to ensure all functionality is workingBackground animations only work if hardware acceleration is enabled and "Reduce motion" flag disabled. The is to avoid animating on the CPU and respect accessibility. We need to ensure animations are not playing in the following scenarios:
--disable-gpu