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

Implement sorting in admin view #1217

Merged
merged 2 commits into from Sep 3, 2023
Merged

Implement sorting in admin view #1217

merged 2 commits into from Sep 3, 2023

Conversation

jonasdeluna
Copy link
Member

@jonasdeluna jonasdeluna commented Sep 3, 2023

Easily extendable to add more sorts. Looks a little ugly, no CSS because I hate it

Screencast.from.09-03-2023.04.56.12.PM.webm

Resolves: ABA-560

@jonasdeluna jonasdeluna requested review from norbye and a team September 3, 2023 14:57
Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

🏅🏅

import {
AlphabeticalComparatorAsc,
AlphabeticalComparatorDesc,
ApplicationComparator,
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this should be imported as a type but oh well

frontend/src/utils/sortAdmissions.ts Outdated Show resolved Hide resolved
@falbru
Copy link
Contributor

falbru commented Sep 3, 2023

LGTM

Copy link
Member

@danielyanghansen danielyanghansen left a comment

Choose a reason for hiding this comment

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

Same comments from as from Ivar, but LGTM

@linear
Copy link

linear bot commented Sep 3, 2023

@jonasdeluna jonasdeluna merged commit a2768d8 into master Sep 3, 2023
2 of 3 checks passed
@jonasdeluna jonasdeluna deleted the order-by-date-sent branch September 3, 2023 15:15
Copy link
Member

@juniwbjerde juniwbjerde left a comment

Choose a reason for hiding this comment

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

Lgtm! 🔥 Just a few remarks:)

Comment on lines 17 to 24
const CreatedAtComparatorAsc: ApplicationComparator = {
compare: (app1, app2) => {
const date1 = new Date(app1.created_at);
const date2 = new Date(app2.created_at);
return date2.getTime() - date1.getTime();
},
description: "Dato Opprettet Stigende",
};
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this just be the negation of theCreatedAtComparatorDesc? Then we wouldn't have as much duplicated code:) The same goes for all the other ones.

},
description: "Dato Opprettet Stigende",
};

Copy link
Member

Choose a reason for hiding this comment

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

I also kind of think I would prefer to have the field sorted on and the direction of the sort as separte options. Like having one seletor for what to sort by, and one for the direction of the sort. Just makes more sense to me. But it's not important, it can be changed later on if we want it:)

juniwbjerde

This comment was marked as duplicate.

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