Optimized the new Welcome UI and style fixes #16164

nullhook · 2022-11-30T07:34:40Z

Resolves brave/brave-browser#26851
Resolves brave/brave-browser#27060
Resolves brave/brave-browser#27061
Resolves brave/brave-browser#27105

This PR optimizes the new Welcome UI and fixes a few style issues:

We use the WebP format for all image assets. This ensures that the asset file size remains small and allows the browser to decode images faster (depending on various factors).
We lazy load specific React components so the initial bundle size is smaller
On initial load, we have a concurrency issue with the background image downloading and decoding versus the browser's layout rendering. This is now solved by waiting for the background image and then rendering the layout.
Specific button colors would adapt to dark/light mode. Since the page has only one theme, the colors are hard-coded for now

Note: Please ensure to test this with --disable-gpu as well

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, npm run lint, 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:

components/brave_welcome_ui/brave_welcome.html

brave-builds · 2022-12-02T21:36:57Z

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

petemill

This looks really nice when I run it locally. Just a couple of questions and very minor changes.

petemill · 2022-12-02T19:20:19Z

components/brave_welcome_ui/components/background/index.tsx

@@ -55,43 +63,65 @@ function AnimatedBackground (props: { children?: JSX.Element }) {
      .to(stars04, { transform: 'scale(1.5)' })

    setScenes({ s1, s2 })
-  }, [])
+  }, [isReadyForAnimation])


The content of the function does not use isReadyForAnimation - should it?

No, the content should not use isReadyForAnimation.

We check for ref.current inside the body of useEffect. If the handle of ref is null then it won't create WAAPI objects.

<S.Box ref={isReadyForAnimation ? ref : null}>

Perhaps that wasn't clear. I should revise this to add another condition. What do you think?

if (!ref.current) return if (!isReadyForAnimation) return

Well either way it needs a comment. But yes it seems clearer to have all the logic for that in 1 place. The ref attribute doesn't need to know whether animation will be used on that ref, so maybe it's clearer to do make that decision based on isReadyForAnimation in in this effect... otherwise we're splitting the logic....

The other option you could consider, which may be better is not using React.useRef for this and instead using React.useCallback((ref) => {... and passing that to the ref prop in the JSX. That will avoid having to use a ref but sync an effect on a prop. You would perform the contents of both effects in the callback when the incoming ref param is truthy.

added anther check. an easier route.

petemill · 2022-12-02T19:20:32Z

components/brave_welcome_ui/components/background/index.tsx

+    lastAnimationEl.addEventListener('finish', () => props.onLoad?.())
+
+    s1.play()
+  }, [isReadyForAnimation])


components/brave_welcome_ui/brave_welcome.html

petemill · 2022-12-06T06:16:35Z

components/brave_welcome_ui/components/select-browser/style.ts

@@ -62,7 +62,7 @@ export const BrowserListBox = styled.div`
    justify-content: center;
    align-items: center;
    flex-direction: column;
-    color: ${(p) => p.theme.color.text01};
+    color: #212529;


Why are we changing some of these SC themed props to css variables and some to direct values?

The text color on this item should be consistent regardless of the theme. A hardcoded value makes sense.

The border color on the item changes based on if the user has selected or not. A css variable here makes sense to manipulate.

I'm not completely sure what you mean here.

In some places in this PR, you have changed from SC theme prop (e.g. p.theme.color.interactive05) to css theme variable (e.g. var(--interactive5)) and in others you have changed from SC theme prop (e.g. p.theme.color.text01) to the value (e.g. #212529). I was wondering what the purpose of that inconsistency is...

var(--interactive5) is used to decorate the checkboxes. I saw this being available in root and utilized it. The hardcoded value #212529 wasn't.

I'm still not sure why sometimes we're using {(p) => p.theme.color.interactive08 (and not --interactive8 from app.global.scss), and sometimes we're using --interactive5 but not p.theme.color.interactive05 and that you've made a conscious decision to change some and not others 🤷

If you don't want the colors to ever change in dark / light then it's best to not use any of the theme variables.

How did you resolve this?

I went with using hardcoded color values.

petemill · 2022-12-06T06:17:30Z

components/brave_welcome_ui/components/select-profile/style.ts

@@ -63,7 +63,7 @@ export const MainBox = styled.div`
 `

 export const ProfileListBox = styled.div`
-  color: ${(p) => p.theme.color.text01};
+  color: #212529;


petemill · 2022-12-06T06:30:42Z

components/brave_welcome_ui/components/welcome/style.ts

+    height: 100%;
+
+    &.initial {
+      scale: 0.9;


What does this achieve? I'm not seeing that there is a transition on this property. Perhaps a comment as to what the purpose of .initial is would help.

When we animate specific styles of an element, we define an initial state. This depends on the type of animation, of course.

The initial state shouldn't be part of the base styles because we perform a check to determine whether animations should play or not. You can see this being expressed below:

<div className={classnames({ 'view-content': true, 'initial': shouldPlayAnimations })}>

Right but there isn't a transition on scale, so does this just jump from 0.9 to 1 when the initial class is removed?

Yes, however, the initial class will never be removed like that.

shouldPlayAnimations computes on prefers reduced motion and load time variable which is during startup and is a one-shot. We also hide with opacity so the element isn't visible.

I'm still not understanding why we have scale: 0.9 or when it will get removed.

Edit: Oh I see it does get animated in the styles contained in the react component. I guess that's why I suggested putting the KeyFrames in this file, close to the initial styles.

components/brave_welcome_ui/components/welcome/index.tsx

components/brave_welcome_ui/main_container.tsx

brave-builds · 2022-12-06T10:55:01Z

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

brave-builds · 2022-12-07T04:43:52Z

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

petemill · 2022-12-07T22:31:22Z

components/brave_welcome_ui/components/background/index.tsx

@@ -55,43 +63,65 @@ function AnimatedBackground (props: { children?: JSX.Element }) {
      .to(stars04, { transform: 'scale(1.5)' })

    setScenes({ s1, s2 })
-  }, [])
+  }, [isReadyForAnimation])


Well either way it needs a comment. But yes it seems clearer to have all the logic for that in 1 place. The ref attribute doesn't need to know whether animation will be used on that ref, so maybe it's clearer to do make that decision based on isReadyForAnimation in in this effect... otherwise we're splitting the logic....

The other option you could consider, which may be better is not using React.useRef for this and instead using React.useCallback((ref) => {... and passing that to the ref prop in the JSX. That will avoid having to use a ref but sync an effect on a prop. You would perform the contents of both effects in the callback when the incoming ref param is truthy.

petemill · 2022-12-07T22:38:01Z

components/brave_welcome_ui/components/select-browser/style.ts

@@ -62,7 +62,7 @@ export const BrowserListBox = styled.div`
    justify-content: center;
    align-items: center;
    flex-direction: column;
-    color: ${(p) => p.theme.color.text01};
+    color: #212529;


In some places in this PR, you have changed from SC theme prop (e.g. p.theme.color.interactive05) to css theme variable (e.g. var(--interactive5)) and in others you have changed from SC theme prop (e.g. p.theme.color.text01) to the value (e.g. #212529). I was wondering what the purpose of that inconsistency is...

petemill · 2022-12-07T22:44:23Z

components/brave_welcome_ui/components/welcome/style.ts

+    height: 100%;
+
+    &.initial {
+      scale: 0.9;


Right but there isn't a transition on scale, so does this just jump from 0.9 to 1 when the initial class is removed?

petemill · 2022-12-07T22:46:02Z

components/brave_welcome_ui/state/hooks.ts

@@ -64,10 +64,10 @@ export function useProfileCount () {
 }

 export function useShouldPlayAnimations () {
-  const [shouldPlayAnimations] = React.useState(
+  const [shouldPlayAnimations] = React.useState(() => (


I think we should have a comment here on why this isn't a module-level variable. Since it never changes, I keep wondering that when I read it. Someone might see this and want to change it.

What is "unexpected behavior and potential bugs"?

Declaring a variable at the module level can make it accessible to all components, which can make it difficult to track changes and test components in isolation. In general, it is better to keep all variables within React's state system, even if they are not expected to change, as this allows React to manage them and ensures that components are able to re-render correctly when state changes.

Declaring a variable at the module level can make it accessible to all components, which can make it difficult to track changes and test components in isolation.

Your hook is already accessible at the module level, accessible to all components, is it not?

export const SHOULD_PLAY_ANIMATIONS = ( loadTimeData.getBoolean('hardwareAccelerationEnabledAtStartup') && !window.matchMedia('(prefers-reduced-motion: reduce)').matches )

is no different in terms of accessibility, except it recognises that it's a constant that doesn't get re-evaluated from every different component which uses it (unlike a hook).

it is better to keep all variables within React's state system, even if they are not expected to change

Why is that? I can understand maybe for tests, but I don't think that applies here.

this allows React to manage them and ensures that components are able to re-render correctly when state changes

If it cannot change then there is no re-render when it changes

Perhaps there was a timing issue calling loadTimeData.getBoolean at the module level (I believe it needs to wait for /strings.js to load first which fills loadTimeData with data)? Or (less likely) perhaps there was an issue calling matchMedia in the module root? If those were the issues, or something similar, then that could be a more meaningful comment.

However, if something like that is the issue, then you can avoid the multiple rendering by having the "constnat" lazy-evaluated on its first call by a react function.

let shouldPlayAnimations: undefined | boolean = undefined export function GetShouldPlayAnimations () { return (shouldPlayAnimations !== undefined) ? shouldPlayAnimations : (shouldPlayAnimations = ( loadTimeData.getBoolean('hardwareAccelerationEnabledAtStartup') && !window.matchMedia('(prefers-reduced-motion: reduce)').matches )) }

changed this to module-level variable

brave-builds · 2022-12-08T16:25:20Z

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

petemill

Please address comments, but functionally everything is good

petemill · 2022-12-09T01:03:31Z

iOS CI looks like a transient failure. I restarted it

brave-builds · 2022-12-09T05:35:48Z

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

* Converted assets to webp format * Lazy loading * Style fixes * Added background image loading handle

kjozwiak · 2022-12-15T06:08:16Z

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.48.57 Chromium: 108.0.5359.128 (Official Build) nightly (64-bit)
-- | --
Revision | 1cd27afdb8e5d057070c0961e04c490d2aca1aa0-refs/branch-heads/5359@{#1185}
OS | Windows 11 Version 22H2 (Build 22621.963)

`Test Case #1` - brave/brave-browser#27060

Went through the STR/Cases outlined via brave/brave-browser#27060 (comment) and ensured that the Skip buttons are now using a lighter colour as per the following:

`Example`	`Example`	`Example`

`Test Case #2` - brave/brave-browser#27061

Went through the STR/Cases outlined via brave/brave-browser#27061 (comment) and verified that the animations under brave://welcome has improved greatly compared to before as per the following:

ensured that brave://welcome loads without any issues/jank when launching a new profile
- ensured that the page rendered correctly and the animations appeared to be smooth
- ensured that the window didn't appear "black" when initially loading brave://welcome
ensured that the animations are smooth when running through the different screens under brave://welcome

New.Tab.-.Brave.2022-12-15.01-01-17.mp4

Created brave/brave-browser#27396 as there's still some issues when resizing the window.

`Test Case #2` - brave/brave-browser#27105

Went through the STR/Cases outlined via brave/brave-browser#27105 (comment) and ensured that all the text via brave://welcome is visible/legible when using a dark theme as per the following:

`Example`	`Example`	`Example`	`Example`	`Example`	`Example`

* Converted assets to webp format * Lazy loading * Style fixes * Added background image loading handle

github-actions bot assigned nullhook Nov 30, 2022

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Nov 30, 2022

nullhook changed the title ~~Converted assets to webp format~~ Optimized the new Welcome UI Nov 30, 2022

nullhook marked this pull request as ready for review December 1, 2022 23:33

nullhook requested a review from petemill December 1, 2022 23:33

petemill reviewed Dec 2, 2022

View reviewed changes

components/brave_welcome_ui/brave_welcome.html Outdated Show resolved Hide resolved

nullhook changed the title ~~Optimized the new Welcome UI~~ Optimized the new Welcome UI and style fixes Dec 2, 2022

nullhook force-pushed the onboarding/optimize branch from 935cd3d to bb8cf67 Compare December 2, 2022 20:24

petemill requested changes Dec 6, 2022

View reviewed changes

nullhook requested a review from petemill December 6, 2022 09:43

nullhook force-pushed the onboarding/optimize branch from 76a53d7 to e77fab6 Compare December 7, 2022 04:00

petemill reviewed Dec 7, 2022

View reviewed changes

petemill approved these changes Dec 9, 2022

View reviewed changes

nullhook added 6 commits December 9, 2022 08:55

Converted assets to webp format

32028a9

Lazy loading

13081d7

Style fixes

41737a6

Added background image loading handle

4ce8861

Resolved feedback

d662b68

Resolved feedback 2

abdff2d

nullhook force-pushed the onboarding/optimize branch from db786ef to abdff2d Compare December 9, 2022 04:25

nullhook merged commit fcf075d into master Dec 9, 2022

nullhook deleted the onboarding/optimize branch December 9, 2022 06:45

github-actions bot added this to the 1.48.x - Nightly milestone Dec 9, 2022

rebron mentioned this pull request Dec 9, 2022

Update onboarding to new design and flow brave/brave-browser#26378

Closed

nullhook added a commit that referenced this pull request Dec 10, 2022

Optimized the new Welcome UI and style fixes (#16164)

96f10e2

* Converted assets to webp format * Lazy loading * Style fixes * Added background image loading handle

nullhook mentioned this pull request Dec 10, 2022

Onboarding fixes (Uplift to 1.47.x) #16321

Merged

7 tasks

kjozwiak mentioned this pull request Dec 15, 2022

improve animation/rendering when resizing brave://welcome brave/brave-browser#27396

Open

nullhook added a commit that referenced this pull request Dec 15, 2022

Optimized the new Welcome UI and style fixes (#16164)

5071645

* Converted assets to webp format * Lazy loading * Style fixes * Added background image loading handle

Optimized the new Welcome UI and style fixes #16164

Optimized the new Welcome UI and style fixes #16164

Conversation

nullhook commented Nov 30, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Dec 2, 2022

petemill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nullhook Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

nullhook Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nullhook Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nullhook Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nullhook Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brave-builds commented Dec 6, 2022

brave-builds commented Dec 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brave-builds commented Dec 8, 2022

petemill left a comment

Choose a reason for hiding this comment

petemill commented Dec 9, 2022

brave-builds commented Dec 9, 2022

kjozwiak commented Dec 15, 2022 • edited Loading

Test Case #1 - brave/brave-browser#27060

Test Case #2 - brave/brave-browser#27061

Test Case #2 - brave/brave-browser#27105

nullhook commented Nov 30, 2022 •

edited

Loading

nullhook Dec 6, 2022 •

edited

Loading

nullhook Dec 6, 2022 •

edited

Loading

nullhook Dec 6, 2022 •

edited

Loading

nullhook Dec 6, 2022 •

edited

Loading

nullhook Dec 7, 2022 •

edited

Loading

kjozwiak commented Dec 15, 2022 •

edited

Loading

`Test Case #1` - brave/brave-browser#27060

`Test Case #2` - brave/brave-browser#27061

`Test Case #2` - brave/brave-browser#27105