Skip to content

Add track selection filter#30

Open
eudes wants to merge 4 commits intoreesvarney:mainfrom
eudes:main
Open

Add track selection filter#30
eudes wants to merge 4 commits intoreesvarney:mainfrom
eudes:main

Conversation

@eudes
Copy link
Copy Markdown

@eudes eudes commented Apr 23, 2024

Hiya!

I found your project recently and would have loved it to have a track filter, so I added it :)

My front-end foo is a bit rusty, and my react is terrible, so feel free to propose changes.
Particularly, the long list of track filters looks quite bad [1], but I'm not sure how to make it a nice scrollable list.

Also, since I was there, I added a docker-compose config to be able to run it self-hosted.

[1]
image

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 23, 2024

@eudes is attempting to deploy a commit to the reesvarney's projects Team on Vercel.

A member of the Team first needs to authorize it.

@reesvarney reesvarney added the enhancement New feature or request label Apr 25, 2024
@reesvarney
Copy link
Copy Markdown
Owner

Hi there, thanks for the contribution!

This looks very promising, I'll try and review it some time over this weekend.

No worries about the layout though, the CSS I used for the filters was quick and rough in the first place, so I'll probably do a general update for that anyway.

Copy link
Copy Markdown
Owner

@reesvarney reesvarney left a comment

Choose a reason for hiding this comment

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

A few suggestions here but looks good mostly :)

If you can address my comments then I'll be happy to merge once I am able to verify that the docker container works for me locally.

Comment on lines +100 to +109
const selectAllTracks = () => {
let tracks = Object.keys(staticData.tracks);
localStorage.setItem("filters", JSON.stringify(tracks))
loadFilters({ track: tracks})
}

const selectNoneTracks = () => {
localStorage.setItem("filters", "{}")
loadFilters({ track: [] })
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think this needs a rework, setting the localstorage will overwrite all other filters as well. What you'd probably want to do is something like (but not exactly):

if(none) {
// Empty for no tracks
filterData.track = [];
} else if(all) {
// All tracks
filterData.track = Object.keys(staticData.tracks);
}
setFilterData(filterData);

Then the useEffect will handle updating the localstorage and updating the servers. (So loadFilters should not be called from anywhere else, I don't mind still separating it out as a function though.)

<span key="track.none" className={styles.filter_button} onClick={selectNoneTracks}>NONE</span>
</span>,
...Object.keys(staticData.tracks).map(id => {
const track = {id,...staticData.tracks[id]};
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

So I'm not that worried about styling however this list may be too large that you cannot close the filter menu on mobile for example. I don't mind doing this part myself but if you fancy a challenge, feel free to try and make it scrollable :)

Comment on lines +112 to +114
const staticData = {tracks};

export default staticData; No newline at end of file
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Probably more readable if this is just a named export as tracks. Then you can use

import {tracks} from "$utils/staticData";

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants