Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Fixes sortTable default sort #14110

Merged
merged 1 commit into from
May 14, 2018
Merged

Fixes sortTable default sort #14110

merged 1 commit into from
May 14, 2018

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented May 13, 2018 •
edited
Loading

Resolves #13721

This PR is basically the same as #13726, I just needed to adjust sortCheck in ledgerTable component.

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).
  • 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. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

  1. start browser on clean profile
  2. enable payments
  3. close browser
  4. run npm run add-simulated-synopsis-visits 500 or 1k
  5. start browser again and make sure that table is sorted by % desc
  6. make sure that there is no CPU spikes and you can click on buttons when on payments page

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@NejcZdovc NejcZdovc added this to the 0.22.x Release 3 (Beta channel) milestone May 13, 2018
@NejcZdovc NejcZdovc self-assigned this May 13, 2018
ryanml
ryanml reviewed May 13, 2018
View reviewed changes
app/renderer/components/preferences/payment/ledgerTable.js Outdated
@@ -272,6 +272,30 @@ class LedgerTable extends ImmutableComponent {
]
}

sortCheck (prevRows, currentRows) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think you could add some tests or perhaps a comment block explaining the logic inside the inner loop? (Perhaps what values of each row are being compared to determine the sort check and why?) It is understandable why the old JSON.stringify comparison of each row may not have been efficient but it was more readable :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comment added

ryanml
ryanml reviewed May 13, 2018
View reviewed changes
app/renderer/components/common/sortableTable.js Outdated
if (this.dimensionCount == null && prevProps.rows) {
for (let i = 0; i < prevProps.rows.length; i++) {
if (this.props.rows[i].length > 0) {
if (this.props.rows[i]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this inner if necessary? the upper check for this.props.rows[i].length > 0 wouldn't pass for falsy objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, not needed

ryanml
ryanml reviewed May 13, 2018
View reviewed changes
Copy link
Contributor

@ryanml ryanml 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.

Test plan works well, no CPU spike, I left a couple of comments

@NejcZdovc
Fixes sortTable default sort
a7bdaac 
Resolves brave#13721

Auditors:

Test Plan:
@NejcZdovc NejcZdovc requested a review from ryanml May 14, 2018 05:56
ryanml
ryanml approved these changes May 14, 2018
View reviewed changes
Copy link
Contributor

@ryanml ryanml left a comment

Choose a reason for hiding this comment

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

Great fix! Thanks for the changes

@bsclifton
Copy link
Member

bsclifton commented May 14, 2018 •
edited
Loading

If you're looking at the list with a bunch of items, a re-sort is done after you delete an item. Unfortunately, this order seems to be different every time.

If you want to delete three items in a row for example, you can't... because it's shuffled after deleting one (because they both have 0 percent)

@bsclifton
Copy link
Member

Perhaps the issue I mentioned above can be added to the scope of #14111

bsclifton
bsclifton approved these changes May 14, 2018
View reviewed changes
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.

Changes work great!

@bsclifton bsclifton merged commit 60ae539 into brave:master May 14, 2018
bsclifton added a commit that referenced this pull request May 14, 2018
@bsclifton
Merge pull request #14110 from NejcZdovc/fix/#13721-sort2
3b5bfea 
Fixes sortTable default sort
@bsclifton
Copy link
Member

bsclifton commented May 14, 2018 •
edited
Loading

master 60ae539
0.23.x 3b5bfea
0.22.x-release3 15d9231

bsclifton added a commit that referenced this pull request May 14, 2018
@bsclifton
Merge pull request #14110 from NejcZdovc/fix/#13721-sort2
15d9231 
Fixes sortTable default sort
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ryanml ryanml ryanml approved these changes

@bsclifton bsclifton bsclifton approved these changes

@jasonrsadler jasonrsadler Awaiting requested review from jasonrsadler

Assignees

@NejcZdovc NejcZdovc

Labels
feature/rewards
Projects
None yet
Milestone
0.22.x Release 3 (Release Channel)
Development

Successfully merging this pull request may close these issues.

Sort ledger table based on %
3 participants
@NejcZdovc @bsclifton @ryanml