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.

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

Sort auto contribute table #988

Closed
NejcZdovc opened this issue Sep 6, 2018 · 5 comments · Fixed by brave/brave-core#487
Closed

Sort auto contribute table #988

NejcZdovc opened this issue Sep 6, 2018 · 5 comments · Fixed by brave/brave-core#487

Comments

@NejcZdovc
Copy link
Contributor

We need to sort table in DESC order based on %.

@ryanml
Copy link
Contributor

ryanml commented Sep 12, 2018

This has been fixed with: brave/brave-core#441

@ryanml ryanml closed this as completed Sep 12, 2018
@NejcZdovc NejcZdovc reopened this Sep 15, 2018
@NejcZdovc
Copy link
Contributor Author

This should be done on DB level and not in JS level. So when you fetch publishers for auto contribute table you should specify order in SQL query using this logic here https://github.com/brave/brave-core/blob/master/components/brave_rewards/browser/publisher_info_database.cc#L366

@ryanml
Copy link
Contributor

ryanml commented Sep 15, 2018

@NejcZdovc will we never have any other type of sorting on this table? It will always be DESC by percentage? If we have plans in the future to provide more sorting options for this, we should receive the initial publisher set (perhaps already in DESC order) and let the client-side handle the sorting of data for speed.

@NejcZdovc
Copy link
Contributor Author

@ryanml we will probably have some sorting, but I don't see this in a near future. When we will have this I suggest that we do generic sorting in table component and pass in default sort as part of header prop for table. This way we just do in once and use it everywhere.

example of header prop

[
 { content: 'Col 1', defaultSort: 'desc', customStyle: {} },
 { content: 'Col 2', customStyle: {} }
]

@LaurenWags
Copy link
Member

LaurenWags commented Oct 3, 2018

Verified passed with

Brave 0.55.10 Chromium: 70.0.3538.22 (Official Build) beta(64-bit)
Revision ac9418ba9c3bd7f6baaffa0b055dfe147e0f8364-refs/branch-heads/3538@{#468}
OS Mac OS X

Using test plan from brave/brave-core#487

Verification passed on

Brave 0.55.11 Chromium: 70.0.3538.35 (Official Build) beta (64-bit)
Revision 28dcb499844fa40c28d5f62e337876cb936f79f5-refs/branch-heads/3538@{#678}
OS Windows 7

Verification Passed on

Brave 0.55.11 Chromium: 70.0.3538.35 (Official Build) beta (64-bit)
Revision 28dcb499844fa40c28d5f62e337876cb936f79f5-refs/branch-heads/3538@{#678}
OS Linux

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment