-
Notifications
You must be signed in to change notification settings - Fork 871
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
Rewards reset profile android #18844
Conversation
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.
Ads changes LGTM++
@@ -297,7 +297,6 @@ export function Settings () { | |||
<BatIcon />{getString('braveRewards')} | |||
</style.title> | |||
{ | |||
!isAndroid && | |||
<style.manageAction> |
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.
Is it possible to remove the enclosing brackets now?
...
</style.title>
<style.manageAction>
...
</style.manageAction>
...
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.
LGTM, just had a nit about the wording.
<message name="IDS_SHOW_ADS_WHEN_BRAVE_IS_NOT_IN_USE" desc="Title for Show Ads when Brave is not in use option in Brave rewards settings."> | ||
Show Ads when Brave is not in use | ||
<message name="IDS_SHOW_ADS_WHEN_BRAVE_IS_NOT_IN_USE_TITLE" desc="Title for Show Ads when Brave is not in use option in Brave rewards settings."> | ||
Brave Notification Ads |
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.
Nit: Is that the correct terminology now? The "notification" part seems superfluous/technical.
A Storybook has been deployed to preview UI for the latest push |
a3f8c2a
to
ddbe59c
Compare
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.
lgtm
{ | ||
!isAndroid && | ||
<style.manageAction> | ||
<style.manageAction> | ||
<button |
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.
Nit: looks like the indenting is off here (through </style.manageAction>
)?
Reset rewards prefs on onCompleteReset
Remove brave rewards preference Refactor rewards prefernce changes
Resolve presubmit check Rebase and Address PR comments Resolve indentation issue
ddbe59c
to
39655ff
Compare
A Storybook has been deployed to preview UI for the latest push |
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.
string reviewers ++
Resolves brave/brave-browser#26902
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
wikinpm run lint
,npm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: