NTP / FTX fix group #9074

petemill · 2021-06-10T06:45:19Z

Fixes for New Tab Page, especially the FTX widget:

FTX: Summary content when there are no balances (fix Add some text to empty summary state brave-browser#16123)
FTX: "Hide widget" menu option will also disconnect auth (fix Hide widget doesn't disconnect the widget brave-browser#16347)
SI: remove clickable wallpapers (fix Follow-up to revert #13190 clickable sponsored image brave-browser#16351)
News: don't show "0" in settings dialog when not yet opted-in (fix Brave News sources list in Customize panel shows 0 when not yet opted-in brave-browser#15977)
FTX widget should not appear when card setting is turned off (fix Brave News sources list in Customize panel shows 0 when not yet opted-in brave-browser#15977)

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

simonhong · 2021-06-11T00:48:59Z

@petemill I filed one more issue about FTX widget - brave/brave-browser#16358
IMO, it would be good to resolve this also because you're trying to resolve most of FTX issues in this PR.
Of course, handling it separately is also nice :)

petemill · 2021-06-11T03:08:03Z

I think I won't solve that one, or will solve it later. I didn't feel we needed to save this as a preference yet:

I think this is a small issue or not an issue at all. It only matters for connecting and authenticating. So if the user doesn't authenticate for that page view then they have to choose the option when they do want to connect. The API calls made when not connected are not affected, so it has no effect.

simonhong · 2021-06-11T03:14:30Z

I think I won't solve that one, or will solve it later. I didn't feel we needed to save this as a preference yet:

I think this is a small issue or not an issue at all. It only matters for connecting and authenticating. So if the user doesn't authenticate for that page view then they have to choose the option when they do want to connect. The API calls made when not connected are not affected, so it has no effect.

I filed that issue after seeing binance widget saves it in prefs.
but I agreed. ftx widget UI doesn't have any difference on that option and it's not used after connecting.

browser/search/ntp_utils.cc

test/BUILD.gn

simonhong · 2021-06-11T04:05:48Z

components/brave_new_tab_ui/containers/newTab/index.tsx

@@ -295,6 +295,7 @@ class NewTabPage extends React.Component<Props, State> {
  }

  toggleShowFTX = () => {
+    this.props.actions.ftx.disconnect()


I should apply this also to cryptodotcom widget :)

browser/search/ntp_utils.cc

…s (due to not opted-in yet)

…of setting all individual widget prefs

simonhong

++

simonhong · 2021-06-11T06:21:11Z

browser/search/ntp_utils.cc

+    VLOG(1) << "Migrating hide widget pref...";
+    // If all the widgets are off, assume user wants no future widgets.
+    bool all_were_off = true;
+    for (auto* const pref_name : kWidgetPrefNames) {


nit: we can use std::all_of here

const bool all_were_off = std::all_of(std::begin(kWidgetPrefNames), std::end(kWidgetPrefNames), [prefs](const char* const pref_names) { ... });

great tip, thanks!

petemill · 2021-06-11T16:59:27Z

Android CI fail known unrelated issue.
Otherwise all CI passed.

srirambv · 2021-06-14T07:25:40Z

Verification passed on

Brave	1.27.60 Chromium: 91.0.4472.101 (Official Build) nightly (x86_64)
Revision	`af52a90bf87030dd1523486a1cd3ae25c5d76c9b-refs/branch-heads/4472@{#1462}`
OS	macOS Version 10.15.7 (Build 19H114)

brave/brave-browser#16123

Verified zero balance text is shown and hidden by default

FTX.Zero.Balance.mov

brave/brave-browser#16347

Verified hiding the widget disconnects the authenticated state

FTX.Hide.widget.mov

brave/brave-browser#16351

Verified only clicking on NTP SI logo has clickthrough

NTP.Click.through.mov

brave/brave-browser#15977

Verified nothing is shown when manually enabling Brave News via customise NTP settings

Brave.News.mov

srirambv · 2021-06-14T07:25:52Z

Verification passed on

Brave	1.27.60 Chromium: 91.0.4472.101 (Official Build) nightly (x86_64)
Revision	`af52a90bf87030dd1523486a1cd3ae25c5d76c9b-refs/branch-heads/4472@{#1462}`
OS	macOS Version 10.15.7 (Build 19H114)

brave/brave-browser#16123

Verified zero balance text is shown and hidden by default

FTX.Zero.Balance.mov

brave/brave-browser#16347

Verified hiding the widget disconnects the authenticated state

FTX.Hide.widget.mov

brave/brave-browser#16351

Verified only clicking on NTP SI logo has clickthrough

NTP.Click.through.mov

brave/brave-browser#15977

Verified nothing is shown when manually enabling Brave News via customise NTP settings

Brave.News.mov

petemill self-assigned this Jun 10, 2021

AlanBreck approved these changes Jun 10, 2021

View reviewed changes

petemill force-pushed the ntp-fixes branch from 0bbb092 to ff3efc1 Compare June 10, 2021 17:50

petemill requested review from simonhong and bsclifton June 10, 2021 17:50

petemill force-pushed the ntp-fixes branch from ff3efc1 to 81f411b Compare June 10, 2021 17:55

simonhong reviewed Jun 11, 2021

View reviewed changes

browser/search/ntp_utils.cc Outdated Show resolved Hide resolved

simonhong reviewed Jun 11, 2021

View reviewed changes

test/BUILD.gn Outdated Show resolved Hide resolved

simonhong reviewed Jun 11, 2021

View reviewed changes

browser/search/ntp_utils.cc Outdated Show resolved Hide resolved

petemill force-pushed the ntp-fixes branch from 81f411b to cb03630 Compare June 11, 2021 05:36

petemill added 2 commits June 10, 2021 22:36

News: Do not show '0' on settings section when there are no publisher…

206b348

…s (due to not opted-in yet)

FTX: UI for summary view with no balances

ec13872

petemill force-pushed the ntp-fixes branch 2 times, most recently from 1507781 to 652a92d Compare June 11, 2021 05:39

petemill added 3 commits June 10, 2021 22:47

NTP: Introduce new "all widgets off" pref and migrate previous style …

3968d7a

…of setting all individual widget prefs

NTP: remove clickable background for branded wallpapers"

d2f77e6

FTX: disconnect when hiding

96cde5b

petemill force-pushed the ntp-fixes branch from 652a92d to 96cde5b Compare June 11, 2021 05:47

simonhong approved these changes Jun 11, 2021

View reviewed changes

petemill merged commit 2e2c4d8 into master Jun 11, 2021

petemill deleted the ntp-fixes branch June 11, 2021 17:00

petemill added this to the 1.27.x - Nightly milestone Jun 11, 2021

petemill added a commit that referenced this pull request Jun 14, 2021

Merge pull request #9074 from brave/ntp-fixes

41afd20

petemill mentioned this pull request Jun 14, 2021

Uplift NTP / FTX fixes to 1.26.x #9107

Merged

rebron mentioned this pull request Jun 16, 2021

FTX widget should not appear when card setting is turned off brave/brave-browser#15972

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NTP / FTX fix group #9074

NTP / FTX fix group #9074

petemill commented Jun 10, 2021 •

edited by rebron

Loading

simonhong commented Jun 11, 2021 •

edited

Loading

petemill commented Jun 11, 2021

simonhong commented Jun 11, 2021

simonhong Jun 11, 2021

simonhong left a comment

simonhong Jun 11, 2021

petemill Jun 11, 2021

petemill commented Jun 11, 2021

srirambv commented Jun 14, 2021

srirambv commented Jun 14, 2021

NTP / FTX fix group #9074

NTP / FTX fix group #9074

Conversation

petemill commented Jun 10, 2021 • edited by rebron Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

simonhong commented Jun 11, 2021 • edited Loading

petemill commented Jun 11, 2021

simonhong commented Jun 11, 2021

simonhong Jun 11, 2021

Choose a reason for hiding this comment

simonhong left a comment

Choose a reason for hiding this comment

simonhong Jun 11, 2021

Choose a reason for hiding this comment

petemill Jun 11, 2021

Choose a reason for hiding this comment

petemill commented Jun 11, 2021

srirambv commented Jun 14, 2021

srirambv commented Jun 14, 2021

petemill commented Jun 10, 2021 •

edited by rebron

Loading

simonhong commented Jun 11, 2021 •

edited

Loading