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.

Sign up for GitHub

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

Jump to bottom

Adds functionality to monthly contribution dropdown (Panel, #2) #1016

Closed
wants to merge 2 commits into from
Closed

Adds functionality to monthly contribution dropdown (Panel, #2) #1016

wants to merge 2 commits into from

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Dec 4, 2018 •
edited

Fixes: brave/brave-browser#2245
Fixes: brave/brave-browser#2823
UI: brave/brave-ui#434

Preview:

Screen Shot 2019-03-20 at 6 52 47 PM
Screen Shot 2019-03-20 at 6 52 57 PM

The brunt of the work for this was adding extension functions to support:

Adding a recurring contribution
Removing a recurring contribution
Retrieving recurring contributions

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

  1. Enable Brave Rewards from a clean profile, add tokens
  2. Navigate to a site such as brave.com
  3. Bring up the wallet panel.
  4. Ensure the monthly contribution dropdown is not shown
  5. Click "Send Tip" and make a monthly donation (mark "Make this monthly")
  6. Re-open the wallet panel
  7. Change monthly contribution amount to a different amount than the recurring tip
  8. Close and re-open panel
  9. Confirm that the value set in step 7 persists
  10. Confirm that the change is reflected in the donation table
  11. Confirm that setting a monthly amount of 0.0 removes the donation from the table.

The above six steps should be tried in different ways with different values. Ex: Ensuring that the data persists across tabs with the same site open

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

@ryanml ryanml self-assigned this Dec 4, 2018
@ryanml ryanml force-pushed the monthly-amount-dropdown branch 2 times, most recently from ba31e98 to 70e8c2a Compare December 12, 2018 15:28
@ryanml ryanml changed the title [WIP] Adds functionality to monthly contribution dropdown (Panel) (#2) Adds functionality to monthly contribution dropdown (Panel) (#2) Dec 12, 2018
jasonrsadler
jasonrsadler previously requested changes Dec 12, 2018
View reviewed changes
Copy link
Contributor

@jasonrsadler jasonrsadler left a comment

Choose a reason for hiding this comment

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

Do npm run sync -- --all and rebase master

components/brave_rewards/browser/extension_rewards_service_observer_browsertest.cc Outdated Show resolved Hide resolved
test/BUILD.gn Outdated Show resolved Hide resolved
@ryanml ryanml force-pushed the monthly-amount-dropdown branch 6 times, most recently from 43ed9c3 to 03beb3d Compare December 13, 2018 21:53
@ryanml
Copy link
Contributor Author

ryanml commented Dec 18, 2018

@jasonrsadler @NejcZdovc this PR has been rebased, tests pass, functionality is the same as it was approved last time.

@ryanml
Copy link
Contributor Author

ryanml commented Mar 20, 2019

@jasonrsadler line length issues resolved

@ryanml
Copy link
Contributor Author

ryanml commented Mar 20, 2019

Referring to the comment about select above, as a part of this PR I am going to implement a styled native component so that we do not deliver this with a clipping issue. cc: @jasonrsadler

@ryanml ryanml changed the title Adds functionality to monthly contribution dropdown (Panel, #2) [WIP] Adds functionality to monthly contribution dropdown (Panel, #2) Mar 20, 2019
@ryanml ryanml requested a review from NejcZdovc March 20, 2019 21:15
@ryanml ryanml changed the title [WIP] Adds functionality to monthly contribution dropdown (Panel, #2) Adds functionality to monthly contribution dropdown (Panel, #2) Mar 20, 2019
@ryanml ryanml force-pushed the monthly-amount-dropdown branch 3 times, most recently from 324cecb to de39cbb Compare March 21, 2019 02:49
NejcZdovc
NejcZdovc reviewed Mar 21, 2019
View reviewed changes
browser/extensions/api/brave_rewards_api.cc Outdated
return RespondNow(Error("Rewards service is not initialized"));
}

rewards_service->GetPublisherBanner(params->publisher_key);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this was not introduced in this PR, but would be awesome if you could add callback to GetPublisherBanner and we can remove observer for it. You would need to change brave_donate_ui GetPublisherBanner as well.

Copy link
Contributor Author

@ryanml ryanml Mar 24, 2019 •
edited

Choose a reason for hiding this comment

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

@NejcZdovc tbh as I was working this out (and it was a bit tricky) I stopped myself to say I think we just need to let this PR move on. The accepted method of passing information from rewards_service_impl -> ledger_impl is changing constantly and is something we should address in separate tickets. Recommend we get this PR wrapped up.

@ryanml @NejcZdovc
Fixes brave/brave-browser#2245
ebdb5b4 
Fixes brave/brave-browser#2823

Monthly donation functionality via panel
@ryanml ryanml mentioned this pull request Mar 25, 2019
Merged
18 tasks
@ryanml
Copy link
Contributor Author

ryanml commented Mar 25, 2019

Closed in favor of: #2069

@ryanml ryanml closed this Mar 25, 2019
@ryanml ryanml deleted the monthly-amount-dropdown branch March 25, 2019 23:13
@stephendonner stephendonner mentioned this pull request Nov 30, 2022
Merged
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NejcZdovc NejcZdovc NejcZdovc left review comments

@bridiver bridiver Awaiting requested review from bridiver

@jasonrsadler jasonrsadler Awaiting requested review from jasonrsadler

Assignees

@ryanml ryanml

Labels
dependency/brave-ui feature/rewards
Projects
None yet
Milestone
0.64.x - Release
6 participants
@ryanml @NejcZdovc @jasonrsadler @bridiver @bbondy @bsclifton