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 Sorting: Sort servers from A-Z or Z-A. #4951

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Nightmaregodss
Copy link

Be able to sort servers from A-Z or Z-A which makes navigating servers easier.

Be able to sort servers from A-Z or Z-A which makes navigating servers easier.
@QuintenQVD0
Copy link
Contributor

IF this is a PR for the current releases then it is the wrong branch develop is v2, 1.0-develop are the current releases

@@ -13,20 +13,43 @@ import useSWR from 'swr';
import { PaginatedResult } from '@/api/http';
import Pagination from '@/components/elements/Pagination';
import { useLocation } from 'react-router-dom';
import Select from '../elements/Select';

Choose a reason for hiding this comment

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

You should be using proper paths, e.g.

import Select from '@/components/elements/Select';

Comment on lines -23 to +25
const uuid = useStoreState(state => state.user.data!.uuid);
const rootAdmin = useStoreState(state => state.user.data!.rootAdmin);
const uuid = useStoreState((state) => state.user.data!.uuid);
const rootAdmin = useStoreState((state) => state.user.data!.rootAdmin);

Choose a reason for hiding this comment

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

Should be reverted I believe, just use prettier

/>
<div css={tw`mx-3`}>

Choose a reason for hiding this comment

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

Shouldn't it be ml-3? How does it currently look?

Copy link
Author

Choose a reason for hiding this comment

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

Looks fine.

Choose a reason for hiding this comment

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

Need a screenshot. Because the switch doesn't have a margin right (without your PR) and you added it to your element

@Boy132
Copy link
Contributor

Boy132 commented Dec 18, 2023

There is already an open PR for this functionality: #3821 (which also adds more sorting modes)

@PadowYT2
Copy link

There is already an open PR for this functionality: #3821 (which also adds more sorting modes)

That's what I wanted to say, but the PR is for sorting servers, not files

@Boy132
Copy link
Contributor

Boy132 commented Dec 18, 2023

There is already an open PR for this functionality: #3821 (which also adds more sorting modes)

That's what I wanted to say, but the PR is for sorting servers, not files

Ah, that's my bad. Then nevermind my comment.

@TekExplorer
Copy link
Contributor

Isn't sorting server-side?

@PadowYT2
Copy link

PadowYT2 commented Dec 18, 2023

Isn't sorting server-side?

There's no sorting in the server list. Well there is, just that it is hardcoded

@PadowYT2
Copy link

Althougjh I don't really like to have this. The current order system will be just messed up

@TekExplorer
Copy link
Contributor

No, I mean that most sorting in the api is done via query parameters and done serverside. this code would need to add sorting options to do this correctly.

Notice here in the application side. This is how sorting is handled, and any pr that adds new sorting options should do the same, or else you will end up with local sorts, which will miss items on the next pages that should now be front and center.

@Nightmaregodss
Copy link
Author

Is this mergable? Or it needs improvements

@TekExplorer
Copy link
Contributor

No. As I said, sorting is supposed to be server-side. Refer to my previous comment

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

Successfully merging this pull request may close these issues.

None yet

5 participants