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

Fixes sortTable default sort #13726

Merged
merged 1 commit into from
Apr 19, 2018
Merged

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Apr 4, 2018

Resolves #13721

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 20
  5. start browser again and make sure that table is sorted by % desc

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 self-assigned this Apr 4, 2018
@NejcZdovc NejcZdovc requested a review from bsclifton April 4, 2018 17:30
@codecov-io
Copy link

codecov-io commented Apr 4, 2018

Codecov Report

Merging #13726 into master will decrease coverage by 0.11%.
The diff coverage is 37.66%.

@@            Coverage Diff             @@
##           master   #13726      +/-   ##
==========================================
- Coverage   56.56%   56.44%   -0.12%     
==========================================
  Files         283      283              
  Lines       28886    28951      +65     
  Branches     4787     4800      +13     
==========================================
+ Hits        16338    16342       +4     
- Misses      12548    12609      +61
Flag Coverage Δ
#unittest 56.44% <37.66%> (-0.12%) ⬇️
Impacted Files Coverage Δ
app/renderer/components/common/sortableTable.js 55.61% <34.72%> (-6.5%) ⬇️
...erer/components/preferences/payment/ledgerTable.js 84.25% <80%> (-1.59%) ⬇️
js/stores/appStoreRenderer.js 91.66% <0%> (-8.34%) ⬇️
app/renderer/components/reduxComponent.js 57.75% <0%> (-3.45%) ⬇️
js/stores/windowStore.js 28.46% <0%> (-0.3%) ⬇️

@jasonrsadler
Copy link
Contributor

screen shot 2018-04-10 at 3 27 48 pm

This works great but if I keep the preferences tab open it doesn't update until I close preferences and reopen

@NejcZdovc NejcZdovc modified the milestones: 0.22.x Release 2, 0.22.x Release 3 (Beta channel) Apr 10, 2018
@NejcZdovc
Copy link
Contributor Author

@jasonrsadler nice find. I think that I need to re-think this one. Because if I add specific default sort row then refresh table is not triggered correctly anymore from tableSort library

@NejcZdovc NejcZdovc removed this from the 0.22.x Release 3 (Beta channel) milestone Apr 11, 2018
@jasonrsadler
Copy link
Contributor

Let me know if I can help

ryanml
ryanml previously approved these changes Apr 19, 2018
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.

This works well and looks good to me!

@NejcZdovc NejcZdovc added this to the 0.22.x Release 3 (Beta channel) milestone Apr 19, 2018
Resolves brave#13721

Auditors:

Test Plan:
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.

This works. The table is updated in realtime now

@NejcZdovc NejcZdovc removed the request for review from bsclifton April 19, 2018 11:14
@NejcZdovc NejcZdovc merged commit 12d502f into brave:master Apr 19, 2018
NejcZdovc added a commit that referenced this pull request Apr 19, 2018
NejcZdovc added a commit that referenced this pull request Apr 19, 2018
@NejcZdovc
Copy link
Contributor Author

master 12d502f
0.23 2cb1beb
0.22 43770d0

NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this pull request May 13, 2018
…sort"

This reverts commit 12d502f, reversing
changes made to 0abdd64.
@NejcZdovc
Copy link
Contributor Author

this PR was reverted in #14106

NejcZdovc added a commit that referenced this pull request May 13, 2018
Revert "Merge pull request #13726 from NejcZdovc/fix/#13721-sort"
NejcZdovc added a commit that referenced this pull request May 13, 2018
Revert "Merge pull request #13726 from NejcZdovc/fix/#13721-sort"
NejcZdovc added a commit that referenced this pull request May 13, 2018
Revert "Merge pull request #13726 from NejcZdovc/fix/#13721-sort"
@NejcZdovc NejcZdovc mentioned this pull request May 13, 2018
10 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sort ledger table based on %
4 participants