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

[UI] Sortable Tables Header By Click #7980

Merged
merged 24 commits into from
Jun 24, 2020

Conversation

6543
Copy link
Member

@6543 6543 commented Aug 26, 2019

Arrow Preview:

how it works ... some notes

  • if table head colum class has attribute sort="xy" -> colum header klickable
  • Klich at colum header for example <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:

@davydov-vyacheslav
Copy link

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?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 26, 2019
@6543 6543 changed the title Sortable Tables [UI] Sortable Tables [UI] [WIP] Aug 26, 2019
@6543
Copy link
Member Author

6543 commented Aug 26, 2019

@davydov-vyacheslav

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 ...
ill test things with paginaton but havent woked with jet so I'll see.

@6543
Copy link
Member Author

6543 commented Aug 26, 2019

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?

yes paginaton and this won't work ... I'll look at the sort function of http://localhost:3000/admin/orgs?sort=alphabetically ...

@codecov-io
Copy link

codecov-io commented Aug 26, 2019

Codecov Report

Merging #7980 into master will decrease coverage by 0.07%.
The diff coverage is 40.99%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
models/error.go 30.50% <0.00%> (ø)
models/issue.go 51.17% <0.00%> (-0.25%) ⬇️
models/issue_label.go 61.60% <0.00%> (-1.45%) ⬇️
models/migrations/migrations.go 4.16% <ø> (ø)
models/migrations/v128.go 0.00% <0.00%> (ø)
models/migrations/v134.go 0.00% <0.00%> (ø)
modules/private/manager.go 0.00% <0.00%> (ø)
routers/repo/compare.go 40.80% <ø> (-0.14%) ⬇️
routers/repo/pull.go 28.21% <0.00%> (-0.83%) ⬇️
routers/repo/pull_review.go 0.00% <0.00%> (ø)
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36d9237...9c27520. Read the comment docs.

@6543
Copy link
Member Author

6543 commented Aug 26, 2019

Current idear ...

  • if class has atribute sorttable -> colum header klickable
  • Klich at colum header for example <ID>
    -> script check if url has already a sort= with <ID>
    YES: open current URL wich sort=<ID> is replaced with <Rev_ID>
    NO: open current URL and add/change sort= to sort=<ID>

@davydov-vyacheslav
Copy link

Current idear ...

  • if class has atribute sorttable -> colum header klickable
  • Klich at colum header for example <ID>
    -> script check if url has already a sort= with <ID>
    YES: open current URL wich sort=<ID> is replaced with <Rev_ID>
    NO: open current URL and add/change sort= to sort=<ID>

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?

@6543
Copy link
Member Author

6543 commented Aug 27, 2019

@davydov-vyacheslav my currend idear to solf this is to tag table header colums
with an sort parameter. sort="alphabetically" and if there exist a reverse then sort="alphabetically,reversealphabetically"
-> https://gitea.com/6543/gitea_sortt

@6543 6543 force-pushed the sortable-tables branch 2 times, most recently from 292ca6e to f9e663b Compare August 27, 2019 16:51
@6543 6543 changed the title Sortable Tables [UI] [WIP] Sortable Tables Header By Click [UI] Aug 27, 2019
@6543
Copy link
Member Author

6543 commented Aug 27, 2019

I'll have a look at the searchParams doku +1

@6543
Copy link
Member Author

6543 commented Aug 28, 2019

@davydov-vyacheslav
ready for merge 🚀 ?

Copy link
Member

@techknowlogick techknowlogick left a 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.

@6543
Copy link
Member Author

6543 commented Aug 28, 2019

@techknowlogick
thanks for the feedback!

the plain HTML way would be the interesting one, but how should links togle betwen normal and revert sort?

@davydov-vyacheslav
Copy link

@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

@6543
Copy link
Member Author

6543 commented Aug 28, 2019

@davydov-vyacheslav I know but it seems you already know a lot about javascript :)

Copy link
Member

@gary-kim gary-kim left a 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.

public/vendor/plugins/sortt/sortt.js Outdated Show resolved Hide resolved
public/vendor/plugins/sortt/sortt.js Outdated Show resolved Hide resolved
public/vendor/plugins/sortt/sortt.js Outdated Show resolved Hide resolved
@6543 6543 changed the title Sortable Tables Header By Click [UI] [WIP] [UI] Sortable Tables Header By Click Sep 3, 2019
@6543 6543 mentioned this pull request Sep 4, 2019
@6543 6543 changed the title [WIP] [UI] Sortable Tables Header By Click [UI] Sortable Tables Header By Click Sep 18, 2019
@6543
Copy link
Member Author

6543 commented Sep 18, 2019

@lafriks now it uses jQuery for events

@lunny @gary-kim

i think it is ready ... 🚀 - any thoughts?

public/vendor/plugins/sortt/sortt.js Outdated Show resolved Hide resolved
public/vendor/plugins/sortt/sortt.js Outdated Show resolved Hide resolved
public/vendor/plugins/sortt/sortt.js Outdated Show resolved Hide resolved
@davydov-vyacheslav
Copy link

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 )

@silverwind
Copy link
Member

Looking good.

Now one thing I kind of dislike here is the single parameter, e.g. sort=alphabetically sort=reversealphabetically. I'd prefer if there were two parameters, one for the sorting column, and one for the direction, which could then map directly to SQL statements ORDER BY ${column} DESC/ASC but I guess that's not something easily supported by the backend, right?

@6543
Copy link
Member Author

6543 commented Jun 23, 2020

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

@silverwind
Copy link
Member

Yeah I guess that's material for another PR 😉

@6543
Copy link
Member Author

6543 commented Jun 23, 2020

@techknowlogick what is your opinion on this?

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 23, 2020
@6543
Copy link
Member Author

6543 commented Jun 23, 2020

@gary-kim you had some opinions on this too - can you dismis or review :)

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: silverwind <me@silverwind.io>
@6543
Copy link
Member Author

6543 commented Jun 23, 2020

@silverwind your solution is not working - :/

@6543
Copy link
Member Author

6543 commented Jun 23, 2020

should I rolle it back or do you have a look at it?

Co-authored-by: silverwind <me@silverwind.io>
@6543
Copy link
Member Author

6543 commented Jun 23, 2020

@silverwind worked :)

and no problem, you have a lot more knowlage about JS than i do - so your suggestions are always welcome 👍

@6543
Copy link
Member Author

6543 commented Jun 23, 2020

I would say let the old pull 🚀

@6543
Copy link
Member Author

6543 commented Jun 24, 2020

ping

@6543
Copy link
Member Author

6543 commented Jun 24, 2020

Ping

@zeripath zeripath merged commit c86478e into go-gitea:master Jun 24, 2020
@6543 6543 deleted the sortable-tables branch June 25, 2020 00:48
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
* [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 <me@silverwind.io>

* Update web_src/js/features/tablesort.js

Co-authored-by: silverwind <me@silverwind.io>

* Update web_src/js/features/tablesort.js

Co-authored-by: silverwind <me@silverwind.io>

Co-authored-by: silverwind <me@silverwind.io>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.