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

Optimized the new Welcome UI and style fixes #16164

Merged
merged 6 commits into from
Dec 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions browser/ui/webui/brave_welcome_ui.cc
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,8 @@ BraveWelcomeUI::BraveWelcomeUI(content::WebUI* web_ui, const std::string& name)
content::GpuDataManager::GetInstance()->HardwareAccelerationEnabled());

profile->GetPrefs()->SetBoolean(prefs::kHasSeenWelcomePage, true);

AddBackgroundColorToSource(source, web_ui->GetWebContents());
}

BraveWelcomeUI::~BraveWelcomeUI() = default;
5 changes: 3 additions & 2 deletions components/brave_welcome_ui/api/web_animation_player.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ class WebAnimationPlayer {
this.timingOption = timingOption
}

to (element: Element | null, keyframeEffectOption: PropertyIndexedKeyframes) {
const kF = new KeyframeEffect(element, keyframeEffectOption, this.timingOption)
to (element: Element | null, keyFrame: PropertyIndexedKeyframes, keyFrameOption?: OptionalEffectTiming) {
const kFOptions = { ...this.timingOption, ...keyFrameOption }
const kF = new KeyframeEffect(element, keyFrame, kFOptions)
const animation = new Animation(kF)
this.animations.push(animation)
return this
Expand Down
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file removed components/brave_welcome_ui/assets/hill.png
Binary file not shown.
Binary file added components/brave_welcome_ui/assets/hill.webp
Binary file not shown.
Binary file removed components/brave_welcome_ui/assets/pyramid.png
Binary file not shown.
Binary file added components/brave_welcome_ui/assets/pyramid.webp
Binary file not shown.
Binary file removed components/brave_welcome_ui/assets/sky.jpg
Binary file not shown.
Binary file added components/brave_welcome_ui/assets/sky.webp
Binary file not shown.
5 changes: 4 additions & 1 deletion components/brave_welcome_ui/brave_welcome.html
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@
#root { height: 100%; width: 100%; }

body {
background: black;
height: 100vh;
margin: 0;
padding: 0;
background: $i18n{backgroundColor};
}
</style>
</head>
Expand Down
93 changes: 62 additions & 31 deletions components/brave_welcome_ui/components/background/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,34 @@

import * as React from 'react'
import * as S from './style'
import classnames from '$web-common/classnames'

import WebAnimationPlayer from '../../api/web_animation_player'
import DataContext from '../../state/context'
import { shouldPlayAnimations } from '../../state/hooks'

import Stars01 from '../svg/stars01'
import Stars02 from '../svg/stars02'
import Stars03 from '../svg/stars03'
import Stars04 from '../svg/stars04'
import fullCompositeBgUrl from '../../assets/[email protected]'
import skyBgUrl from '../../assets/sky.webp'

interface BackgroundProps {
children?: JSX.Element
static: boolean
onLoad?: () => void
}

function AnimatedBackground (props: { children?: JSX.Element }) {
function Background (props: BackgroundProps) {
const ref = React.useRef<HTMLDivElement>(null)
const { setScenes } = React.useContext(DataContext)
const [hasLoaded, setHasLoaded] = React.useState(false)
const isReadyForAnimation = hasLoaded && !props.static

React.useEffect(() => {
if (!ref.current) return
if (!isReadyForAnimation) return

const s1 = new WebAnimationPlayer()
const s2 = new WebAnimationPlayer()
Expand Down Expand Up @@ -55,43 +63,66 @@ function AnimatedBackground (props: { children?: JSX.Element }) {
.to(stars04, { transform: 'scale(1.5)' })

setScenes({ s1, s2 })
}, [])
}, [isReadyForAnimation])
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@nullhook nullhook Dec 6, 2022

Choose a reason for hiding this comment

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

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}>

Copy link
Contributor Author

@nullhook nullhook Dec 6, 2022

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added anther check. an easier route.


React.useEffect(() => {
if (!ref.current) return
if (!isReadyForAnimation) return

const s1 = new WebAnimationPlayer()
const starsContainer = ref.current.querySelector('.stars-container')
const hillsContainer = ref.current.querySelector('.hills-container')

s1.to(starsContainer, { opacity: 1 }, { delay: 250 })
.to(hillsContainer, { opacity: 1 }, { delay: 250 })

const lastAnimationEl = s1.animations[s1.animations.length - 1]
lastAnimationEl.addEventListener('finish', () => props.onLoad?.())

s1.play()
}, [isReadyForAnimation])
Copy link
Member

Choose a reason for hiding this comment

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

ditto


const handleImgLoad = () => {
setHasLoaded(true)

// When animations are disabled, we trigger onLoad instantly
if (!shouldPlayAnimations) {
props.onLoad?.()
}
}

return (
<S.Box ref={ref}>
<div className="stars-container">
<Stars01 />
<Stars02 />
<Stars03 />
<Stars04 />
</div>
<S.Box ref={isReadyForAnimation ? ref : null}>
{isReadyForAnimation && (
<div className="stars-container">
<Stars01 />
<Stars02 />
<Stars03 />
<Stars04 />
</div>
)}
<div className="content-box">
{props.children}
</div>
<div className="sky" />
<div className="hills-container">
<div className="hills-base hills03"></div>
<div className="hills-base hills02"></div>
<div className="hills-base hills01"></div>
<div className="pyramid"></div>
</div>
<img
// We animate the background image via CSS only.
className={classnames({
'background-img': true,
'is-visible': hasLoaded
})}
src={!props.static ? skyBgUrl : fullCompositeBgUrl}
onLoad={handleImgLoad}
/>
{isReadyForAnimation && (
<div className="hills-container">
<div className="hills-base hills03"></div>
<div className="hills-base hills02"></div>
<div className="hills-base hills01"></div>
<div className="pyramid"></div>
</div>
)}
</S.Box>
)
}

function Background (props: BackgroundProps) {
if (!props.static) {
return (<AnimatedBackground>
{props.children}
</AnimatedBackground>
)
}

return (
<S.StaticBackground>
{props.children}
</S.StaticBackground>
)
}

export default Background
107 changes: 51 additions & 56 deletions components/brave_welcome_ui/components/background/style.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,38 +4,77 @@
// you can obtain one at https://mozilla.org/MPL/2.0/.

import styled from 'styled-components'
import skyeBgUrl from '../../assets/sky.jpg'
import hillBgUrl from '../../assets/hill.png'
import pyramidBgUrl from '../../assets/pyramid.png'
import backgroundUrl2x from '../../assets/[email protected]'

export const Box = styled.div`
import hillBgUrl from '../../assets/hill.webp'
import pyramidBgUrl from '../../assets/pyramid.webp'

export const Box = styled.div`
.content-box {
position: fixed;
width: 100%;
height: 100vh;
height: 100%;
z-index: 999;

display: flex;
align-items: center;
justify-content: center;
}

.sky {
.background-img {
position: fixed;
width: 100vw;
height: 100vh;
background: url(${skyeBgUrl}) no-repeat;
background-size: cover;
width: 100%;
height: 100%;
z-index: 1;
object-fit: cover;
opacity: 0;
transition: opacity .2s ease-in;

&.is-visible {
opacity: 1;
}
}

.hills-container {
position: fixed;
width: 100vw;
height: 100vh;
height: 100%;
z-index: 2;
opacity: 0;
}

.stars-container {
position: fixed;
width: 100vw;
height: 100%;
z-index: 50;
opacity: 0;

svg {
width: 100%;
height: auto;
position: absolute;
transform-origin: center;
}

.stars01 {
bottom: 0;
transform: scale(1.14);
filter: blur(3px);
}

.stars02 {
top: 10%;
}

.stars03 {
top: 15%;
}

.stars04 {
top: 30%;
transform: scale(0.8);
opacity: 0;
}
}

.hills-base {
Expand Down Expand Up @@ -76,48 +115,4 @@ export const Box = styled.div`
z-index: 1;
transform: translateX(20%);
}

.stars-container {
position: fixed;
width: 100vw;
height: 100vh;
z-index: 50;

svg {
width: 100%;
height: auto;
position: absolute;
transform-origin: center;
}

.stars01 {
bottom: 0;
transform: scale(1.14);
filter: blur(3px);
}

.stars02 {
top: 10%;
}

.stars03 {
top: 15%;
}

.stars04 {
top: 30%;
transform: scale(0.8);
opacity: 0;
}
}
`

export const StaticBackground = styled.div`
display: flex;
align-items: center;
justify-content: center;
width: 100%;
height: 100vh;
background: url(${backgroundUrl2x}) no-repeat center center;
background-size: cover;
`
4 changes: 2 additions & 2 deletions components/brave_welcome_ui/components/help-improve/style.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,10 @@ export const Grid = styled.div`
width: 22px;
height: 22px;
border-radius: 4px;
border: 1px solid ${(p) => p.theme.color.interactive08};
border: 1px solid #AEB1C2;

&:checked {
accent-color: ${(p) => p.theme.color.interactive05};
accent-color: #4C54D2;
}
}
`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,10 @@ import * as React from 'react'

import * as S from './style'
import DataContext from '../../state/context'
import { useShouldPlayAnimations } from '../../state/hooks'
import { shouldPlayAnimations } from '../../state/hooks'

function ImportInProgress () {
const { scenes } = React.useContext(DataContext)
const shouldPlayAnimations = useShouldPlayAnimations()
const ref = React.useRef<HTMLDivElement>(null)

React.useEffect(() => {
Expand Down
46 changes: 46 additions & 0 deletions components/brave_welcome_ui/components/loader/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Copyright (c) 2022 The Brave Authors. All rights reserved.
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// you can obtain one at https://mozilla.org/MPL/2.0/.

import * as React from 'react'
import styled from 'styled-components'
import { LoaderIcon } from 'brave-ui/components/icons'

const Loading = styled('div')`
width: 100%;
height: 100%;
display: flex;
align-items: center;
justify-content: center;
`

const Graphic = styled('div')`
width: 50px;
height: 50px;
align-self: center;

svg {
fill: white;
}
`

export default function Loader () {
const [show, setShow] = React.useState(false)

React.useEffect(() => {
const timerId = setTimeout(() => setShow(true), 100)

return () => {
clearTimeout(timerId)
}
}, [])

return (
<Loading aria-busy='true'>
<Graphic aria-label='Loading'>
{ show ? <LoaderIcon /> : null }
</Graphic>
</Loading>
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@nullhook nullhook Dec 6, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

How did you resolve this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with using hardcoded color values.

border: 0;
box-shadow: 0 0 0 4px var(--border-color);
position: relative;
Expand Down Expand Up @@ -97,4 +97,8 @@ export const ActionBox = styled.div`
display: flex;
grid-gap: 10px;
margin-bottom: 40px;

button {
color: white;
}
`
Loading