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

server-side sorting of admin page #4722

Merged
merged 8 commits into from Mar 8, 2024
Merged

Conversation

minrk
Copy link
Member

@minrk minrk commented Mar 6, 2024

removes all in-page sort, which removes sort by admin, server name, sort by running, as those are not available from the API.

This builds on #4720 and #4721 to use the new sort parameter in the GET /users API and persist it in the new view state.

This means sort, etc. are always global, no matter how many users there are (closes #3816)

Running column switches from sort to filter, matching the ?state query parameter in the API (closes #4482). So while you can't show running servers first, you can show only running servers.

needed some CSS on the column widths to avoid jumps when toggling active server filter.

demo:

admin-sort.mov

draft because I haven't updated the tests, but it works

removes in-page sort, which removes sort by server name, sort by running

Running column switches from sort to filter, matching the `?state` query parameter in the API

needs some CSS on the column widths to avoid jumps when toggling active servers
@minrk minrk added the new new features label Mar 6, 2024
avoids imperfect logic detecting `?`
next to name filter, so it's not in the table headings

merges Running & Actions columns,
since it's really just Actions now (server actions & user actions)
server model needs high-level User object for `progress_url` (it probably shouldn't)
@minrk
Copy link
Member Author

minrk commented Mar 8, 2024

I moved the active filter next to the name filter. There's lots more room up there.

As a result, I merged the 'Running' and 'Actions' column, since there wasn't anything 'Running' about it as it's not a sort column anymore.

Screenshot 2024-03-08 at 09 51 04

This lets the table flow and resize a bit nicer, because the button 'cell' actually spans 4 columns, so it doesn't get squished as much on narrow screens:

Screenshot 2024-03-08 at 09 55 32

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, having "only " before in the label makes the function of the added checkbox very clear, and we can afford to be verbose now that we have space where its now located - nice!

I've attempted to do code review on these parts as well and didn't find something that seemed weird - but I also didn't have time to make sure I understood the details with full confidence. Adding tests remain, and you are on that already so I'll go for an approve!

4px margin matches 8px cell padding (margin is added on both sides)

and center when buttons collapse to single column
@minrk
Copy link
Member Author

minrk commented Mar 8, 2024

alignment was bugging me, here's the buttons on a narrow screen now:

Screenshot 2024-03-08 at 10 19 46

Arguably, they should collapse into a hamburger menu, but that's one step too far for the level of refactoring I want to do here.

@minrk minrk marked this pull request as ready for review March 8, 2024 12:06
@minrk minrk merged commit bfe143f into jupyterhub:main Mar 8, 2024
19 checks passed
@minrk minrk deleted the sort-order-admin-ui branch March 8, 2024 12:10
@minrk
Copy link
Member Author

minrk commented Mar 8, 2024

Thanks @consideRatio!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new new features
Projects
None yet
2 participants