Update "Sponsored Images" toggle UI #14986

sangwoo108 · 2022-09-03T12:02:33Z

Rough requirements:

The toggle UI should be below background image options
The new toggle UI should contain descriptions about Sponsored Images
When Brave Rewards or Brave Ad are not enabled, a button should be visible
When a user toggles on the option and Reward & Ads are enabled, the description
should notify the user that they're earning from it.
When clicking the "Rewards" button, "Reward tour" UI appears or "Brave Ads" option is enabled.

Light theme

Dark theme

Resolves brave/brave-browser#25197

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:

Manual: Described from PR description

brave-builds · 2022-09-03T14:17:18Z

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

fallaciousreasoning

Mostly looks good, couple of comments about CSS variables

components/brave_new_tab_ui/containers/newTab/settings/backgroundImage.tsx

components/brave_new_tab_ui/containers/newTab/settings/sponsoredImagesToggle.tsx

fallaciousreasoning · 2022-09-05T02:32:54Z

components/brave_new_tab_ui/containers/newTab/settings/sponsoredImagesToggle.tsx

+  justify-content: space-between;
+  padding: 12px 14px;
+  border-radius: 10px;
+  background-color: ${p => isDarkTheme(p) ? css`#1E2029` : css`white`};


Suggested change

background-color: ${p => isDarkTheme(p) ? css`#1E2029` : css`white`};

background-color: var(--background1);

should automatically handle light/dark mode (and you can remove isDarkTheme)

See brave/components/web-components/app.global.scss for the CSS variables we have. I think there's a convention for transforming the color name in Figma to the css variable name, but there are a bunch of missing colors in there sadly.

convention for transforming the color name in Figma to the css variable name

Great! Is there a doc for this part? I was wondering how we manage all these variable stuffs. Also this is my first time to use Figma, It'd be really nice to learn best practices.

I haven't really used it much either 😆 I think in the new design system the Figma ==> CSS variable transformation is more straightforward. I often just search for the hex color in app.global.css or the last bit of the color name in figma but without leading 0s (ie. Light Theme/Text/text01 becomes --text1).

@petemill is probably the best person to ask about this.

Theme/Text/text01 becomes --text1).

Aha! that's the convention! thanks a lot :)

components/brave_new_tab_ui/containers/newTab/settings/sponsoredImagesToggle.tsx

brave-builds · 2022-09-06T03:25:25Z

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

diracdeltas · 2022-09-06T07:08:04Z

probably @PrivacyMatters needs to take a look here to review the strings and overall opt-in UI

PrivacyMatters · 2022-09-06T09:15:24Z

@sangwoo108 It's important to use correct terminology. The images show the toggle set to ON and which makes it an opt-out (not an opt-in). If the toggle was off, and the user turned it ON, it would be opt-in.

The default chosen by Brave is that Sponsored Images are ON by default (so an opt-out), even when Brave Rewards have NOT been enabled by a user.

Has it been implemented differently in South Korea?

sangwoo108 · 2022-09-06T10:30:18Z

@sangwoo108 It's important to use correct terminology. The images show the toggle set to ON and which makes it an opt-out (not an opt-in). If the toggle was off, and the user turned it ON, it would be opt-in.

The default chosen by Brave is that Sponsored Images are ON by default (so an opt-out), even when Brave Rewards have NOT been enabled by a user.

Has it been implemented differently in South Korea?

Oh, my bad. There are no changes intended default state of sponsored image. I just update UI of the toggle UI.

updated pr description and commit message

sangwoo108 · 2022-09-06T10:39:40Z

cc. @rebron since strings are from spec.

brave-builds · 2022-09-06T11:19:26Z

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

PrivacyMatters · 2022-09-06T12:07:15Z

@sangwoo108 is this is about providing users who do not have Rewards enabled with a notice that they can earn BAT by enabling it?

components/web-components/app.global.scss

sangwoo108 · 2022-09-07T00:28:07Z

@sangwoo108 is this is about providing users who do not have Rewards enabled with a notice that they can earn BAT by enabling it?

@PrivacyMatters Yes, I think so. This is part of brave/brave-browser#15252 and you can find the spec for this there. cc. @rebron

brave-builds · 2022-09-07T01:54:10Z

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

sangwoo108 · 2022-09-13T00:19:11Z

Hi, @PrivacyMatters . Do you have anything else for me to address?

Rough requirements: - The toggle UI should be below background image options - The new toggle UI should contain descriptions about Sponsored Images - When Brave Rewards or Brave Ad are not enabled, a button should be visible - When a user opts in the option and Reward & Ads are enabled, the description should notify the user that they're earning from it. - When clicking the "Rewards" button, "Reward tour" UI appears or "Brave Ads" option is enabled.

brave-builds · 2022-09-16T04:38:01Z

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

PrivacyMatters · 2022-09-16T11:14:41Z

@sangwoo108 no more comment from me. It is only being presented to people with the SI default ON.

sangwoo108 · 2022-09-16T12:09:33Z

Thank you so much @PrivacyMatters !

@fallaciousreasoning Could you take another look please?

fallaciousreasoning

LGTM, sorry for the delay @sangwoo108

sangwoo108 · 2022-09-20T00:11:14Z

LGTM, sorry for the delay @sangwoo108

No problem at all. You're one of the most responsive reviewers around me so that I count on 😎 Thanks for the review!

I checked reviewers checklist on behalf of you as it's blocking.

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

github-actions bot assigned sangwoo108 Sep 3, 2022

sangwoo108 force-pushed the sko/ntp-si branch 3 times, most recently from be67258 to d81cf9c Compare September 3, 2022 13:15

sangwoo108 requested review from fallaciousreasoning, simonhong and petemill September 4, 2022 22:43

fallaciousreasoning reviewed Sep 5, 2022

View reviewed changes

sangwoo108 force-pushed the sko/ntp-si branch from d81cf9c to c78d781 Compare September 6, 2022 02:40

sangwoo108 commented Sep 6, 2022

View reviewed changes

components/brave_new_tab_ui/containers/newTab/settings/sponsoredImagesToggle.tsx Outdated Show resolved Hide resolved

components/brave_new_tab_ui/containers/newTab/settings/sponsoredImagesToggle.tsx Show resolved Hide resolved

sangwoo108 closed this Sep 6, 2022

sangwoo108 reopened this Sep 6, 2022

sangwoo108 changed the title ~~Update "Sponsored Images" opt-in UI~~ Update "Sponsored Images" toggle UI Sep 6, 2022

sangwoo108 force-pushed the sko/ntp-si branch from c78d781 to eb1a2e5 Compare September 6, 2022 10:38

sangwoo108 requested a review from rebron September 6, 2022 10:39

fallaciousreasoning reviewed Sep 6, 2022

View reviewed changes

components/web-components/app.global.scss Outdated Show resolved Hide resolved

sangwoo108 added 2 commits September 16, 2022 12:32

Cleanup: use color variable, convention

5ec4be6

Cleanup: use more global color variable, typo

485bf65

sangwoo108 force-pushed the sko/ntp-si branch from 646dbce to 485bf65 Compare September 16, 2022 03:32

sangwoo108 requested a review from fallaciousreasoning September 16, 2022 12:09

fallaciousreasoning approved these changes Sep 19, 2022

View reviewed changes

sangwoo108 merged commit 74da03e into master Sep 20, 2022

sangwoo108 deleted the sko/ntp-si branch September 20, 2022 00:13

github-actions bot added this to the 1.45.x - Nightly milestone Sep 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update "Sponsored Images" toggle UI #14986

Update "Sponsored Images" toggle UI #14986

sangwoo108 commented Sep 3, 2022 •

edited

Loading

brave-builds commented Sep 3, 2022

fallaciousreasoning left a comment

fallaciousreasoning Sep 5, 2022

fallaciousreasoning Sep 5, 2022

sangwoo108 Sep 6, 2022

fallaciousreasoning Sep 6, 2022 •

edited

Loading

sangwoo108 Sep 7, 2022

brave-builds commented Sep 6, 2022

diracdeltas commented Sep 6, 2022

PrivacyMatters commented Sep 6, 2022

sangwoo108 commented Sep 6, 2022 •

edited

Loading

sangwoo108 commented Sep 6, 2022

brave-builds commented Sep 6, 2022

PrivacyMatters commented Sep 6, 2022

sangwoo108 commented Sep 7, 2022 •

edited

Loading

brave-builds commented Sep 7, 2022

sangwoo108 commented Sep 13, 2022

brave-builds commented Sep 16, 2022

PrivacyMatters commented Sep 16, 2022

sangwoo108 commented Sep 16, 2022

fallaciousreasoning left a comment

sangwoo108 commented Sep 20, 2022 •

edited

Loading

	background-color: ${p => isDarkTheme(p) ? css`#1E2029` : css`white`};
	background-color: var(--background1);

Update "Sponsored Images" toggle UI #14986

Update "Sponsored Images" toggle UI #14986

Conversation

sangwoo108 commented Sep 3, 2022 • edited Loading

Light theme

Dark theme

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Sep 3, 2022

fallaciousreasoning left a comment

Choose a reason for hiding this comment

fallaciousreasoning Sep 5, 2022

Choose a reason for hiding this comment

fallaciousreasoning Sep 5, 2022

Choose a reason for hiding this comment

sangwoo108 Sep 6, 2022

Choose a reason for hiding this comment

fallaciousreasoning Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

sangwoo108 Sep 7, 2022

Choose a reason for hiding this comment

brave-builds commented Sep 6, 2022

diracdeltas commented Sep 6, 2022

PrivacyMatters commented Sep 6, 2022

sangwoo108 commented Sep 6, 2022 • edited Loading

sangwoo108 commented Sep 6, 2022

brave-builds commented Sep 6, 2022

PrivacyMatters commented Sep 6, 2022

sangwoo108 commented Sep 7, 2022 • edited Loading

brave-builds commented Sep 7, 2022

sangwoo108 commented Sep 13, 2022

brave-builds commented Sep 16, 2022

PrivacyMatters commented Sep 16, 2022

sangwoo108 commented Sep 16, 2022

fallaciousreasoning left a comment

Choose a reason for hiding this comment

sangwoo108 commented Sep 20, 2022 • edited Loading

sangwoo108 commented Sep 3, 2022 •

edited

Loading

fallaciousreasoning Sep 6, 2022 •

edited

Loading

sangwoo108 commented Sep 6, 2022 •

edited

Loading

sangwoo108 commented Sep 7, 2022 •

edited

Loading

sangwoo108 commented Sep 20, 2022 •

edited

Loading