fix: keep runner list sorting across pages, make page size configurable - #39161
fix: keep runner list sorting across pages, make page size configurable#39161bircni wants to merge 1 commit into
Conversation
GiteaBot
added
the
lgtm/need 2
bircni
changed the title
GiteaBot
added
lgtm/need 1
|
Move. it to the bottom near the pages? |
| htmlDoc := NewHTMLParser(t, resp.Body) | ||
|
|
||
| // the chosen page size is reflected in the per-page selector | ||
| assert.Positive(t, htmlDoc.Find(`.dropdown .menu a.item.active[href*="limit=25"]`).Length()) |
There was a problem hiding this comment.
First, the active is abused. active doesn't mean selected, you can take a look at the source code.
Second, I really doubt about the value of such TestActionsRunnerListPagination test. It only queries some links on the page loosely, while the links are all built by framework AddParamFromRequest and QueryBuild. I don't see what kind of regression bugs the test would catch in the future.
There was a problem hiding this comment.
I believe we need this first: refactor: pagination/pager - #39162
Then the page links are always correct. It doesn't make sense to keep testing all the links again and again for all pages.
|
Simple backport: fix: add missing query parameters on runner list page - #39163 |
| // runnersPageSizes are the selectable "runners per page" values on the runner management page. | ||
| var runnersPageSizes = []int{25, 50, 100, 200} | ||
|
|
||
| const runnersDefaultPageSize = 100 |
There was a problem hiding this comment.
Here also comes a question: what benefits does the "selectable items per page" bring?
Isn't it "the more the better" in most cases?
There was a problem hiding this comment.
so what do you suggest? showing always all?
There was a problem hiding this comment.
I mean maybe a default 50 or 100 is good enough? No need to make it "selectable". We never did so on other pages. If we'd really like to make the "limit" selectable, we need a plan to consider all pages.
There was a problem hiding this comment.
It just annoyed me yesterday as I have 102 runners 😂
There was a problem hiding this comment.
I could agree 100 seems too many ...
So reduce it to 50?
There was a problem hiding this comment.
OK, I misunderstood. Maybe you mean you need a large limit to see all?
From UI/UX perspective, I don't see why a list should show too many items, it exceeds human processing capabilities. When the list is large, usually it needs to support "search" or "filter".
And, Gitea already has a lot of config options under [ui] [ui.admin] [ui.user] etc, some preference settings are stored in local storage, etc. There were also discussions about "adding per-user setting", etc.
So overall there are already many different approaches. If we don't have a plan, and the problem is trivial, I'd prefer to keep things simple until we a design which is universally applicable to most pages, instead of ad-hoc "improvements"
There was a problem hiding this comment.
Actually I also have a question: now you have 102 runners and would like to add "limit=200", then some days later, if you have 202 or 1002 runners, would you add "limit=300/500/2000" then?
There was a problem hiding this comment.
Totally agree - maybe instead of having a lot of config options for pagination we could add one global which triggers for all.
Just an idea...
5 participants
The runner management page (repo / org / user / admin) lost the active
sort and search filter when switching pages: the paginator was never
given the request query params, so page links were bare
?page=N. Thepage size was also hard-coded to 100.
size persist across pages.
default 100), driven by a validated
limitquery param.