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, #3 origin) #2069

Merged
merged 2 commits into from
Apr 8, 2019
Merged

Adds functionality to monthly contribution dropdown (Panel, #3 origin) #2069

merged 2 commits into from
Apr 8, 2019

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Mar 25, 2019 •
edited
Loading

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

Original PR was here: #1016 (moved from fork to origin)

Preview:

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

Screen Shot 2019-03-25 at 4 09 29 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 added this to the 0.64.x - Nightly milestone Mar 25, 2019
@ryanml ryanml self-assigned this Mar 25, 2019
@ryanml ryanml mentioned this pull request Mar 25, 2019
Closed
18 tasks
@ryanml ryanml requested a review from NejcZdovc March 26, 2019 08:17
NejcZdovc
NejcZdovc previously requested changes Mar 26, 2019
View reviewed changes
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

something is not working:

image

also I know that you extended dropdown, but now ( 0 is visible. Can we add empty space before number so that it always behave as 100 amount?

image

@ryanml ryanml force-pushed the monthly-amount-dropdown-2 branch 3 times, most recently from c4ea838 to 3e0cbaa Compare March 28, 2019 05:16
@ryanml ryanml requested a review from NejcZdovc March 28, 2019 05:18
@ryanml
Copy link
Contributor Author

ryanml commented Mar 28, 2019 •
edited
Loading

@NejcZdovc the issue with dropdown amounts in your photo should be fixed now.

@ryanml ryanml force-pushed the monthly-amount-dropdown-2 branch 2 times, most recently from 4cc619d to e152908 Compare April 1, 2019 19:15
@ryanml ryanml force-pushed the monthly-amount-dropdown-2 branch 3 times, most recently from d52cd4a to d8eb1f9 Compare April 1, 2019 20:58
NejcZdovc
NejcZdovc reviewed Apr 2, 2019
View reviewed changes
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

should we remove line when you select 0 as you just removed recurring? I think it would make sense as we remove it from rewards page tabel as well. wdyt?

NejcZdovc
NejcZdovc previously requested changes Apr 2, 2019
View reviewed changes
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

STR:

  • have two windows (in one rewards page, in second some site)
  • add recurring and leave panel open
  • remove recurring from rewards page
  • panel still shows recurring option

NejcZdovc
NejcZdovc reviewed Apr 2, 2019
View reviewed changes
package.json Outdated Show resolved Hide resolved
@NejcZdovc
Copy link
Contributor

please rebase to the latest master as CI is failing

@ryanml ryanml force-pushed the monthly-amount-dropdown-2 branch 2 times, most recently from a81aa95 to 016648d Compare April 3, 2019 21:47
@ryanml ryanml requested a review from NejcZdovc April 3, 2019 21:50
@ryanml ryanml dismissed NejcZdovc’s stale review April 3, 2019 21:50

that case is now covered

@ryanml ryanml force-pushed the monthly-amount-dropdown-2 branch from 016648d to b615ce7 Compare April 5, 2019 19:00
@NejcZdovc NejcZdovc force-pushed the monthly-amount-dropdown-2 branch 5 times, most recently from 04cfd66 to c9234bb Compare April 8, 2019 10:36
@ryanml @NejcZdovc
Fixes brave/brave-browser#2245
2b3debd 
Fixes brave/brave-browser#2823

Monthly donation functionality via panel
@ryanml ryanml force-pushed the monthly-amount-dropdown-2 branch from 47678af to 9898879 Compare April 8, 2019 11:33
NejcZdovc
NejcZdovc previously approved these changes Apr 8, 2019
View reviewed changes
@ryanml ryanml force-pushed the monthly-amount-dropdown-2 branch from cec37f8 to 5d9baec Compare April 8, 2019 21:43
@ryanml ryanml requested a review from tmancey April 8, 2019 21:44
@tmancey
Copy link
Collaborator

tmancey commented Apr 8, 2019

LGMT as only change since @NejcZdovc approved was updating commit hash for related brave-ui PR

@ryanml ryanml merged commit 8746c0c into master Apr 8, 2019
@ryanml ryanml deleted the monthly-amount-dropdown-2 branch April 8, 2019 23:44
This was referenced May 15, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmancey tmancey tmancey approved these changes

@NejcZdovc NejcZdovc NejcZdovc left review comments

@jasonrsadler jasonrsadler Awaiting requested review from jasonrsadler

Assignees

@ryanml ryanml

Labels
dependency/brave-ui feature/rewards
Projects
None yet
Milestone
0.65.x - Release
3 participants
@ryanml @NejcZdovc @tmancey