Skip to content
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

Add setting options to toggle NTP widgets #2762

Merged
merged 5 commits into from
Jul 3, 2019
Merged

Conversation

imptrx
Copy link
Contributor

@imptrx imptrx commented Jun 20, 2019

Spec: brave/brave-browser#4510 (Current scope of this PR does not include right clicking on the widgets to open up a mini-menu)
Brave-ui Sister PR: brave/brave-ui#503
Quick View:
toggle-widgets

Submitter Checklist:

Test Plan:

  1. Open new tabs page with a fresh profile - all NTP elements should appear
  2. Open settings menu from the settings icon - 3 new settings should exist below show background
  3. Toggle the three options; show clock; show stats; show top sites - they should disappear/reappear on the page
  4. Settings should persist after browser is closed and reopened

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@bsclifton
Copy link
Member

bsclifton commented Jun 25, 2019

@imptrx is there an issue captured that this PR fixes?
nevermind! just saw it: brave/brave-browser#4510

cezaraugusto
cezaraugusto previously approved these changes Jun 26, 2019
Copy link
Contributor

@cezaraugusto cezaraugusto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm adding @petemill as reviewer who suggested moving stored data to be in settings instead of using localStorage. I'm ok either doing that as a follow-up or holding this one a bit until we have the proper api exposed

@petemill
Copy link
Member

@cezaraugusto I'm going to PR the change to either this, or to master if this is merged first. I'm happy with either scenario but if this is merged first, any users who change the setting will have their setting reset back to default once the prefstore change lands

@petemill
Copy link
Member

Pushed a rebase on master. We'll still want to update brave-ui sha with the result of merging brave/brave-ui#503 when that happens

@petemill
Copy link
Member

Here's a branch which adds these remaining toggles to the pref store, for integrating perhaps by @imptrx when #2831 is merged to master and this is rebased. The branch is a rebase of this PR on that, and then adds the prefs.
https://github.com/brave/brave-core/compare/toggle-on-profile-prefs

@cezaraugusto
Copy link
Contributor

set PR as blocked until #2831 is resolved

cezaraugusto
cezaraugusto previously approved these changes Jul 3, 2019
Copy link
Contributor

@cezaraugusto cezaraugusto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ great Peter work here! thanks team this is super cool. 💯

bsclifton
bsclifton previously approved these changes Jul 3, 2019
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great! 😄 Thanks @petemill for the example code RE: preferences. Awesome job, @imptrx!

Besides the code looking good and verifying that it works, I also verified that the new settings are persisted in the Default\Preferences file as expected 👍
Screen Shot 2019-07-03 at 11 03 40 AM

@bsclifton
Copy link
Member

Don't forget to add a test plan and link to it from brave/brave-browser#4510

@imptrx imptrx dismissed stale reviews from bsclifton and cezaraugusto via 1bc5078 July 3, 2019 18:06
@imptrx imptrx force-pushed the toggle-widgets-ntp branch 2 times, most recently from 6fca56c to b613ed6 Compare July 3, 2019 18:20
@imptrx imptrx merged commit 325532b into master Jul 3, 2019
@imptrx imptrx deleted the toggle-widgets-ntp branch July 3, 2019 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants