Updates NTP's Sponsored Images toggle functionality. #16895

mkarolin · 2023-01-27T20:33:42Z

Resolves brave/brave-browser#27304

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

brave-builds · 2023-01-27T21:55:43Z

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

sangwoo108

This looks good to me! But you might want to wait for reviews from @petemill or @fallaciousreasoning

fallaciousreasoning

LGTM, with a few (mostly optional) nits

fallaciousreasoning · 2023-01-29T19:41:34Z

components/brave_new_tab_ui/containers/newTab/index.tsx

@@ -301,7 +302,10 @@ class NewTabPage extends React.Component<Props, State> {

  startRewards = () => {
    const { rewardsState } = this.props.newTabData
-    if (!rewardsState.rewardsEnabled) {
+    if (!rewardsState.rewardsEnabled ||
+      (rewardsState.externalWallet &&


Optional nit:

Suggested change

(rewardsState.externalWallet &&

rewardsState.externalWallet?.status === mojom.WalletStatus.kConnected

fallaciousreasoning · 2023-01-29T19:42:42Z

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

-                  canSupportAds={!!this.props.newTabData.rewardsState.adsSupported}/>
+                  canSupportAds={!!this.props.newTabData.rewardsState.adsSupported}
+                  isExternalWalletConnected={
+                    !!this.props.newTabData.rewardsState.externalWallet &&


Optional nit:

Suggested change

!!this.props.newTabData.rewardsState.externalWallet &&

this.props.newTabData.rewardsState.externalWallet?.status !== mojom.WalletStatus.kNotConnected

fallaciousreasoning · 2023-01-29T19:46:06Z

components/brave_new_tab_ui/containers/newTab/settings/icons/sponsoredImageInfo.tsx

+
+import * as React from 'react'
+
+export function SponsoredImageInfoIcon () {


Optional:

This doesn't need to be a functional component as it doesn't take any props, so you can just assign it to a constant:

const SponsoredImageInfoIcon = <svg ....> .... </svg>

And then use it like this:

import SponsoredImageInfoIcon from '...' // . // . // . return <div> {SponsoredImageInfoIcon} </div>

Semi-related question: If we had a lot of components using this pattern in our codebase, does it negatively affect performance for sessions which don't result in uses of those components. Instead of exporting functions that don't get called, there would instead be a lot of React.createClass calls during initial JS module parsing. A single (or a few hundred??) instances of this of course won't matter, but it would be interesting to know if this is a best practice and at what point it causes negative impact.

fallaciousreasoning · 2023-01-29T20:04:08Z

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

+  return getLocale('sponsoredImageOnDescription')
+}
+
+function GetDescriptionText (rewardsEnabled: boolean,


Nit: I think functions should be lower case (except for components) to fit in with most other stuff. This is pretty inconsistently enforced though (sorry)

Suggested change

function GetDescriptionText (rewardsEnabled: boolean,

function getDescriptionText (rewardsEnabled: boolean,

fallaciousreasoning · 2023-01-29T20:04:40Z

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

 `

-export default function SponsoredImageToggle ({ onChange, onEnableRewards, checked, disabled, rewardsEnabled: rewardEnabled, adsEnabled, canSupportAds }: Props) {
-  const showRewardButton = checked && !disabled && (!rewardEnabled || (!adsEnabled && canSupportAds))
+function GetInfoTooltipText (checked: boolean, rewardsEnabled: boolean) {


Nit: I think functions should be lower case (except for components) to fit in with most other stuff. This is pretty inconsistently enforced though (sorry)

Suggested change

function GetInfoTooltipText (checked: boolean, rewardsEnabled: boolean) {

function getInfoTooltipText (checked: boolean, rewardsEnabled: boolean) {

fallaciousreasoning · 2023-01-29T20:04:56Z

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

+  return getLocale('sponsoredImageOnAdsOff')
+}
+
+function GetButtonText (rewardsEnabled: boolean,


Nit: I think functions should be lower case (except for components) to fit in with most other stuff. This is pretty inconsistently enforced though (sorry)

Suggested change

function GetButtonText (rewardsEnabled: boolean,

function getButtonText (rewardsEnabled: boolean,

mkarolin · 2023-01-30T14:21:39Z

Thanks @fallaciousreasoning, all great points. Addressed in 23d9e1a

brave-builds · 2023-01-30T16:24:23Z

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

mkarolin · 2023-01-31T14:20:34Z

Squashed commits.

brave-builds · 2023-01-31T14:58:14Z

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

petemill · 2023-01-31T18:41:02Z

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

+    display: none;
+  }
+
+  &:hover .tooltip {


This won't show for non-mouse users. I think it could be pretty trivial to have the tooltip show when the icon is focused. Could you try?

petemill

Just some issues with positioning in rtl and keyboard activation

petemill · 2023-01-31T18:45:39Z

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

+  .tooltip {
+    position: absolute;
+    bottom: 100%;
+    left: -200px;


In RTL UI (which you can force and still keep english via chrome://flags), this is making the text get clipped

This could get fixed by the following (as I can't think of a better way right now!):

[dir=rtl] & { right: -200px; }

Unfortunately that puts the arrow in the wrong place, so that also needs repositioning similarly.

@petemill inset-inline-start seems to work for both directions. WDYT?

@petemill It also looks like toggles' bullet doesn't move correctly to the ON position with RTL set.

brave-builds · 2023-01-31T22:54:06Z

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

mkarolin · 2023-02-06T17:19:54Z

@petemill, could you, please, take another look. Thanks

mkarolin · 2023-02-14T00:32:46Z

Updated screenshots:

LTR	RTL

petemill

Thanks for addressing!

mkarolin requested review from petemill, sangwoo108 and zenparsing January 27, 2023 20:33

mkarolin self-assigned this Jan 27, 2023

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Jan 27, 2023

sangwoo108 requested a review from fallaciousreasoning January 28, 2023 02:45

sangwoo108 approved these changes Jan 28, 2023

View reviewed changes

fallaciousreasoning approved these changes Jan 29, 2023

View reviewed changes

zenparsing approved these changes Jan 30, 2023

View reviewed changes

Updates NTP's Sponsored Images toggle functionality.

8330e4e

mkarolin force-pushed the maxk-ntp-settings-si-toggle-update branch from 23d9e1a to 8330e4e Compare January 31, 2023 14:20

petemill reviewed Jan 31, 2023

View reviewed changes

petemill requested changes Jan 31, 2023

View reviewed changes

Review fixes.

47c9d7a

mkarolin requested a review from petemill February 3, 2023 20:23

petemill approved these changes Feb 14, 2023

View reviewed changes

mkarolin merged commit 4262787 into master Feb 14, 2023

mkarolin deleted the maxk-ntp-settings-si-toggle-update branch February 14, 2023 01:35

mkarolin added this to the 1.50.x - Nightly milestone Feb 14, 2023

	(rewardsState.externalWallet &&
	rewardsState.externalWallet?.status === mojom.WalletStatus.kConnected

	!!this.props.newTabData.rewardsState.externalWallet &&
	this.props.newTabData.rewardsState.externalWallet?.status !== mojom.WalletStatus.kNotConnected


		import * as React from 'react'

		export function SponsoredImageInfoIcon () {

	function GetDescriptionText (rewardsEnabled: boolean,
	function getDescriptionText (rewardsEnabled: boolean,

	function GetInfoTooltipText (checked: boolean, rewardsEnabled: boolean) {
	function getInfoTooltipText (checked: boolean, rewardsEnabled: boolean) {

	function GetButtonText (rewardsEnabled: boolean,
	function getButtonText (rewardsEnabled: boolean,

Updates NTP's Sponsored Images toggle functionality. #16895

Updates NTP's Sponsored Images toggle functionality. #16895

Uh oh!

Conversation

mkarolin commented Jan 27, 2023

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Uh oh!

brave-builds commented Jan 27, 2023

Uh oh!

sangwoo108 left a comment

Choose a reason for hiding this comment

Uh oh!

fallaciousreasoning left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkarolin commented Jan 30, 2023

Uh oh!

brave-builds commented Jan 30, 2023

Uh oh!

mkarolin commented Jan 31, 2023

Uh oh!

brave-builds commented Jan 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petemill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brave-builds commented Jan 31, 2023

Uh oh!

mkarolin commented Feb 6, 2023

Uh oh!

mkarolin commented Feb 14, 2023

Uh oh!

petemill left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!