-
Notifications
You must be signed in to change notification settings - Fork 877
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
Migrate crypto widgets visibility in NTP #11266
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fix brave/brave-browser#19708 Migration is done once in the first NTP loading. For new users, all crypto widgets are hidden by default. For existing users, if current foremost widget is crypto widget, only it is visible and others are hidden. After migration, user can control crypto widget's visibility from gallery. To make all crypto widgets hidden by default, each pref's default value is not changed. Their default prefs is still "true". As their visibility are managed by prefs and widget order is managed in local storage, it's difficult to start migration from c++. If default is changed to false, it's very difficult to determine whether foremost widget is crypto or not in the NTP webui.
simonhong
force-pushed
the
ntp_crypto_widget
branch
from
November 24, 2021 07:33
c33daf8
to
da101b5
Compare
simonhong
changed the title
WIP: Migrate crypto widgets visibility in NTP
Migrate crypto widgets visibility in NTP
Nov 24, 2021
Start widget visibility migation when ftx auth state is fetched.
Start widget visibility migation when ftx auth state is fetched.
petemill
reviewed
Nov 25, 2021
simonhong
force-pushed
the
ntp_crypto_widget
branch
from
November 25, 2021 01:14
e488600
to
ba78700
Compare
simonhong
force-pushed
the
ntp_crypto_widget
branch
from
November 25, 2021 01:26
ba78700
to
91e9a37
Compare
petemill
approved these changes
Nov 25, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice fast and clean work. 😄
Would be great to squash the commits if you have time.
Used squash & merge option! |
simonhong
added a commit
that referenced
this pull request
Nov 25, 2021
* Migrate crypto widgets visibility in NTP fix brave/brave-browser#19708 Migration is done once in the first NTP loading. For new users, all crypto widgets are hidden by default. For existing users, if current foremost widget is crypto widget, only it is visible and others are hidden. And, authed widgets are visible regardless of its position in widget stack After migration, user can control crypto widget's visibility from gallery. To make all crypto widgets hidden by default, each pref's default value is not changed. Their default prefs is still "true". As their visibility are managed by prefs and widget order is managed in local storage, it's difficult to start migration from c++. If default is changed to false, it's very difficult to determine whether foremost widget is crypto or not in the NTP webui.
This was referenced Nov 25, 2021
Verification passed on
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix brave/brave-browser#19708
Migration is done in the first NTP loading only once.
For new users, all crypto widgets are hidden by default.
For existing users, if current foremost widget is crypto widget,
only it is visible and others are hidden.
After migration, user can control crypto widget's visibility from gallery.
To make all crypto widgets hidden by default, each pref's default
value is not changed. Their default prefs is still "true".
As their visibility are managed by prefs and widget order is managed in local
storage, it's difficult to start migration from c++.
If user has already authed crypto widgets, they will be also visible regardless of position in the widget stack.
Resolves
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
Test 1 (New user)
Test 2-1 (Existing user)
Test 2-2 (Existing user)
Test 2-3 (Existing user that has authed widget)