-
Notifications
You must be signed in to change notification settings - Fork 889
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
Welcome Page: Move from brave-ui to brave-core #2896
Conversation
2eac9fd
to
b42e1fe
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.
Code changes all LGTM! Just waiting on my build to finish to manually verify all the on-boarding functionality 💯
I'm going to add the 'imported from' prefix as some of these commits don't mention the welcome page |
277db49
to
b6b983b
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.
Works great. I needed to rebase on master to review, so I also added the prefix to the commit messages and fixed the spelling on another one.
I did add some nits regarding some import paths which unnecessarily traverse the FS. They are minor but stops the directory being fully self-contained. Will leave it up to you @cezaraugusto as if it's ever a problem it will be apparent and could easily be fixed at the time.
@@ -13,7 +13,7 @@ import { | |||
SkipButton, | |||
FooterButton, | |||
Bullet | |||
} from 'brave-ui/features/welcome' | |||
} from '../../../../components/brave_welcome_ui/components' |
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: exits UI folder then goes back in - can we replace ../../../../components/brave_welcome_ui
with ../../
?
|
||
// Images | ||
import { WelcomeRewardsImage } from 'brave-ui/features/welcome/images' | ||
import { WelcomeRewardsImage } from '../../../../components/brave_welcome_ui/components/images' |
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.
Same nit for paths
|
||
// Images | ||
import { WelcomeSearchImage } from 'brave-ui/features/welcome/images' | ||
import { WelcomeSearchImage } from '../../../../components/brave_welcome_ui/components/images' |
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.
Same nit for paths
|
||
// Utils | ||
import { getLocale } from '../../../common/locale' | ||
|
||
// Images | ||
import { WelcomeShieldsImage } from 'brave-ui/features/welcome/images' | ||
import { WelcomeShieldsImage } from '../../../../components/brave_welcome_ui/components/images' |
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.
Same nit for paths
|
||
// Images | ||
import { WelcomeThemeImage } from 'brave-ui/features/welcome/images' | ||
import { WelcomeThemeImage } from '../../../../components/brave_welcome_ui/components/images' |
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.
Same nit for paths
|
||
// Shared components | ||
import { ArrowRightIcon } from 'brave-ui/components/icons' | ||
|
||
// Images | ||
import { WelcomeLionImage } from 'brave-ui/features/welcome/images' | ||
import { WelcomeLionImage } from '../../../../components/brave_welcome_ui/components/images' |
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.
Same nit for paths
@@ -4,7 +4,7 @@ | |||
|
|||
import { Dispatch } from 'redux' | |||
import { getSearchEngineProvidersSuccess, getBrowserProfilesSuccess } from './actions/welcome_actions' | |||
import { State as ImportBoxState } from '../brave_welcome_ui/components/screens/importBox' | |||
import { State as ImportBoxState } from '../brave_welcome_ui/containers/screens/importBox' |
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: path exits UI and then goes back in - can just be ./
b6b983b
to
f863a3e
Compare
good catch, thanks @petemill. updated |
f863a3e
to
b64592b
Compare
…e` props to `customStyle`
… spec - this removes legacy theme file and other goodies
…e-specific compnents
-hover states on secondary button updates, this was actually to button component code and not theme but was intentional changes - started incorporating the brave palette when necessary - updated bg svg to fill more of the space - adjusted select component to brave palette and further from the purple aesthetic to a more netural form component
- small padding adjustments to button components, more left right space - added lettering spacing to button component. letters were running into eachother - updated all teh graphics to a more standard size and svg - vertically aligned the content in the panel. i don't think this inline css styling of the panel is the best way this should be done but it's the idea and i saw other css there - deleted pngs - adjusted bg svg color from bright orange to more of a rewards color - adjusted wording in some locale - the button component for a green "success" state on active might bea good addition to the button components. we have "warn" but no success alternative. -
- adjusted caps text to sentence case like our other standard buttons - adjusted the background svg back down to 600-ish pixels - adjusted the button icons to fall more in line with 4px scale increments
- updated to latest approved copy from Brad R. - adjusted one button accent - animation updates i had previously had staged and forgot to commit
- the text change of the button didn't make sense until we have a dropdown present so i changed both states to the same
-ease updates
- met in the middle for animation timing but a little on the faster side. these panels are big though and require some time to transition without feeling like it's jumping.
svg asset and text
…nding on region defaults
…ton into local state
…Card since it had overriden almost all properties
40658bb
to
d95463f
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.
🎆 🍾 🍾 🍾 🎆
I squashed the file-moving and the import-path-moving commits since they're doing the same thing.
Thank you!!
address brave/brave-browser#2335
close brave/brave-browser#5216
Test Plan