NTP: Add settings section for Dashboard options #5151
Conversation
cezaraugusto
force-pushed
the
ca-8769
branch
2 times, most recently
from
8da4192
to
08eb7d9
Compare
cezaraugusto
changed the title
cezaraugusto
changed the title
| checked={brandedWallpaperOptIn} | ||
| // This option can only be enabled if | ||
| // users opt in for background images | ||
| checked={showBackgroundImage && brandedWallpaperOptIn} |
There was a problem hiding this comment.
See https://bravesoftware.slack.com/archives/CQGTGBNH1/p1585937176040100 per @karenkliu comment users can't see branded images if background images are turned off so here I'm deliberately turning branded wallpapers off when a user opt-out form default background images. If they regret, it falls back to the branded wallpaper setting.
There was a problem hiding this comment.
@karenkliu , I'm curious if user turns off the bg option, sponsored images option should have same option value.
IMO, just disabling it seems fine because its value is not changed by turning bg option off. WDYT?
If you think this is fine from UX point of view, I'm also fine with this PR's change.
There was a problem hiding this comment.
@simonhong The way we have it set up right now, turning off the Background Images toggle also turns off Sponsored Images and makes the toggle disabled until Background Images is turned back on. Settings should match that behavior.
In the future, we would like to give users more background options, so the Background Images toggle could have more sub-toggles that are independent of each other like this:
Background Images on/off
- Brave default images on/off
- Sponsored images on/off
- Set your own custom images on/off
Turning off background images would turn off all the sub-toggles, but turning off Brave default images could be independent of sponsored images and not affect that toggle.
There was a problem hiding this comment.
Turning off background images would turn off all the sub-toggles, but turning off Brave default images could be independent of sponsored images and not affect that toggle
Ok - this PR meets above requirements. Thanks for explanation!
| // If background image setting is not turned ON, | ||
| // inform the back-end to also disable the branded wallpaper setting. | ||
| // We will later disable interacting with the button as well. | ||
| if (isBackgroundEnabled === false) { |
There was a problem hiding this comment.
See comment in https://github.com/brave/brave-core/pull/5151/files#r403256591. Branded wallpapers are visually disabled and with this change also turned off deliberately if default background wallpapers are turned off.
|
@bridiver could you take a look? Merging is blocked on you as a code owner. |
|
deleted |
|
Rebased on latest master with lint error fix. |
|
All green, merging. Thanks all! |
Close brave/brave-browser#8760
Submitter Checklist:
npm run lint)git rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
Background imagestoggle is OFF,Sponsored imagesshould be disabled.Reviewer Checklist:
After-merge Checklist:
changes has landed on.