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

[feat-216] support for sorting maps/songs and playlists #601

Merged
merged 10 commits into from
Jan 11, 2025

Conversation

silentrald
Copy link
Contributor

@silentrald silentrald commented Oct 2, 2024

Closes #216, Closes #329, Closes #574

UI change as of e3d27e4
image

@silentrald silentrald changed the title [feat-216] support for sorting maps/songs [feat-216] support for sorting maps/songs and playlists Oct 7, 2024
@silentrald silentrald force-pushed the feat/216 branch 3 times, most recently from c3fe24d to 2e4db8c Compare October 16, 2024 01:14
Copy link

@@ -182,22 +282,43 @@ export function MapsPlaylistsPanel({ version, isActive }: Props) {
<BsmDropdownButton className="h-full relative z-[1] flex justify-center" buttonClassName="flex items-center justify-center h-full rounded-full px-2 py-1 whitespace-nowrap" icon="filter" text="pages.version-viewer.maps.search-bar.filters-btn" textClassName="whitespace-nowrap" withBar={false}>
{(
tabIndex === 0 ? (
<FilterPanel className="absolute top-[calc(100%+3px)] origin-top w-[500px] h-fit p-2 rounded-md shadow-md shadow-black" filter={mapFilter} onChange={setMapFilter} />
<FilterPanel className="absolute top-[calc(100%+3px)] origin-top w-[500px] h-fit p-2 rounded-full shadow-md shadow-black" filter={mapFilter} onChange={setMapFilter} />
Copy link
Owner

Choose a reason for hiding this comment

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

image

) : (
<LocalPlaylistFilterPanel className="absolute top-[calc(100%+3px)] origin-top w-[300px] h-fit p-2 rounded-md shadow-md shadow-black" filter={playlistFilter} onChange={setPlaylistFilter} />
<LocalPlaylistFilterPanel className="absolute top-[calc(100%+3px)] origin-top w-[300px] h-fit p-2 rounded-full shadow-md shadow-black" filter={playlistFilter} onChange={setPlaylistFilter} />
Copy link
Owner

Choose a reason for hiding this comment

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

image

onClick={handleAscendingClick}
/>

<BsmSelect
Copy link
Owner

Choose a reason for hiding this comment

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

For playlists, the NPS sort is way too long, so instead of Notes per Second, use NPS.
Also, I think we can remove Max and Min and keep only NPS. If users want playlists with the minimum NPS, they can simply reverse the sort 🤔
image

import { Comparator, Comparison } from "shared/models/comparator.type";
import { BsmLocalMap } from "shared/models/maps/bsm-local-map.interface";

export class MapsSorterService {
Copy link
Owner

Choose a reason for hiding this comment

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

I think this class doesn't need to be a Service; it should be a regular class
Since this class can be used by both the back-end and front-end, you can place it in ./shared/models/maps/
If you want to avoid duplicating comparators in memory, you can use a static variable. 😉

import { Comparator, Comparison } from "shared/models/comparator.type";
import { LocalBPListsDetails } from "shared/models/playlists/local-playlist.models";

export class PlaylistsSorterService {
Copy link
Owner

Choose a reason for hiding this comment

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

Same as MapsSorterService

Copy link
Owner

Choose a reason for hiding this comment

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

You can even create an abstract class for these two 🤔
In the future, it's possible we might want to sort other types of content (models, etc, who knows), and we could create another Sorter class extending the abstract class 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was thinking along the lines of just of using a default sorter class since the two classes literally just share the same logic, just different variable values. The class can still be extended if we need any special thing for the sorter to do in any case.

Copy link
Owner

@Zagrios Zagrios left a comment

Choose a reason for hiding this comment

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

Looks good to me 🙌

@Zagrios
Copy link
Owner

Zagrios commented Jan 11, 2025

Thanks @silentrald ! ❤️
Merging 🚀

@Zagrios Zagrios merged commit 1301bfe into Zagrios:master Jan 11, 2025
5 checks passed
@silentrald silentrald deleted the feat/216 branch January 11, 2025 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants