-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[UI] Sortable Tables Header By Click #7980
Conversation
am I correct that this library is for client-side sorting? If so, we may have issues with pagination as library know nothing about pagination, haven't we? |
it is client-side sorting. I just searchend how to add this feature without many dependencys and so on ... |
yes paginaton and this won't work ... I'll look at the sort function of http://localhost:3000/admin/orgs?sort=alphabetically ... |
Codecov Report
@@ Coverage Diff @@
## master #7980 +/- ##
==========================================
- Coverage 43.43% 43.36% -0.08%
==========================================
Files 593 594 +1
Lines 83286 83453 +167
==========================================
+ Hits 36178 36186 +8
- Misses 42620 42781 +161
+ Partials 4488 4486 -2
Continue to review full report at Codecov.
|
Current idear ...
|
I like this idea, but I don't think that all the table's controller has sort order for all the columns of these tables, at least at UI you are not able to perform sort by each column. Does controller support sort order for each column of the table? |
@davydov-vyacheslav my currend idear to solf this is to tag table header colums |
292ca6e
to
f9e663b
Compare
I'll have a look at the searchParams doku +1 |
@davydov-vyacheslav |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contribution :)
A few pieces of feedback:
- If JS is getting injected into pages it should be in the footer with all other JS (and done in a DRY way)
- Instead of doing onclick events, you can bind to elements in JS itself (this lets all related code be related to each other), and if you need to use arguments you can use something such as data-* attributes
- librejs files need to be updated for each new JS dependency
- This dependency is so small it could be included in the application itself
That being said, this can be done without JS, and have plain HTML links be generated, which is the way that I would recommend.
@techknowlogick the plain HTML way would be the interesting one, but how should links togle betwen normal and revert sort? |
@6543 I'm not maintainer, so, globally, my opinion cost nothing :) But the code looks much better than original one and I don't have more concerns about it |
@davydov-vyacheslav I know but it seems you already know a lot about javascript :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution.
I think we should also think about if we could just do this with html though.
I have one more comment for this feature. Not all the columns' headers are able to sort. It would be nice to get images of current sorted column and the direction of sort (something like this one ) |
Looking good. Now one thing I kind of dislike here is the single parameter, e.g. |
@silverwind since there wan no nameconvention used for the reverse sort of a sort option there is no option to find it out unless we refactor the whole sort code of all pages witch use this kind of sort ... |
Yeah I guess that's material for another PR 😉 |
@techknowlogick what is your opinion on this? |
@gary-kim you had some opinions on this too - can you dismis or review :) |
Co-authored-by: silverwind <[email protected]>
Co-authored-by: silverwind <[email protected]>
@silverwind your solution is not working - :/ |
should I rolle it back or do you have a look at it? |
Co-authored-by: silverwind <[email protected]>
@silverwind worked :) and no problem, you have a lot more knowlage about JS than i do - so your suggestions are always welcome 👍 |
I would say let the old pull 🚀 |
ping |
Ping |
* [UI] Sortable Tables Header By Click * get rid of padding above header * restart CI * fix lint * convert getArrow JS to SortArrow go func * addopt SortArrow funct * suggestions from @silverwind - tablesort.js Co-authored-by: silverwind <[email protected]> * Update web_src/js/features/tablesort.js Co-authored-by: silverwind <[email protected]> * Update web_src/js/features/tablesort.js Co-authored-by: silverwind <[email protected]> Co-authored-by: silverwind <[email protected]>
Arrow Preview:
how it works ... some notes
<ID>
-> script check if url has already a sort= with
<ID>
NO: if data-sortt indikates default set URLsort=
YES: set URLsort variable
-> script check if URLsort and sort attribute is same
YES: -> check if reverse attribute exist
YES: generate URL with reverse sort param and open
NO: generate URL with sort param and open
Roadmap: