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

LB-1727: Filters for link listens page #3145

Merged
merged 13 commits into from
Jan 29, 2025
Merged

Conversation

granth23
Copy link
Contributor

Problem

LB1727: Filters for link listens page

Solution

FIlter.webm

Filtering functionality added

Action

  • Let me know if any additional details or corrections are required.
  • If the changes look good, kindly proceed with merging the PR.

@granth23 granth23 marked this pull request as draft January 23, 2025 18:02
@granth23 granth23 marked this pull request as ready for review January 23, 2025 18:08
@MonkeyDo MonkeyDo changed the title LB1727: Filters for link listens page LB-1727: Filters for link listens page Jan 24, 2025
@anshg1214 anshg1214 self-requested a review January 26, 2025 08:18
Copy link
Member

@anshg1214 anshg1214 left a comment

Choose a reason for hiding this comment

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

The PR Looks good. However you can perform cleanup by using bootstrap classes.

frontend/js/src/settings/link-listens/LinkListens.tsx Outdated Show resolved Hide resolved
frontend/css/filter-search.less Outdated Show resolved Hide resolved
frontend/js/src/settings/link-listens/LinkListens.tsx Outdated Show resolved Hide resolved
@granth23 granth23 requested a review from anshg1214 January 27, 2025 10:17
Copy link
Member

@anshg1214 anshg1214 left a comment

Choose a reason for hiding this comment

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

One final change, and we're good to go.

frontend/js/src/settings/link-listens/LinkListens.tsx Outdated Show resolved Hide resolved
Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Nice work so far, it's working quite nicely!
I think searching by entity type was the right choice, otherwise it would get quite confusing I think.

Currently missing:

  • a button to clear the search (currently emptying the search bar you can't click on the search button)
  • another state variable to hold the original unlinkedListensProps, which is what we should use as the searchable list. If you link a track, it is removed from the list of options (see for example in the openMultiTrackMappingModal function). We should be able to hold state of the modified list while also beign able to filter it.
  • a "no results" message to show users

frontend/js/src/settings/link-listens/LinkListens.tsx Outdated Show resolved Hide resolved
Comment on lines 109 to 112
if (!searchQuery) {
setUnlinkedListens(unlinkedListensProps);
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Currently I think this code path will never be hit, because the submit button will be disasbled if there are less that X characters.
However that also means we probably need a "clear search" button or a system that will detect char length in the textarea onChange callback, and set the unfiltered list.
Not sure what is the best way to handle this, what are your thoughts? clear search button, or always-enabled submit button?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The last commit didn't have the below code segment
if (!searchQuery) { setUnlinkedListens(unlinkedListensProps); return; }
So that has been catered to already, a clear search button leading to the unfiltered Link Listens is a must so will make the changes.

frontend/js/src/settings/link-listens/LinkListens.tsx Outdated Show resolved Hide resolved
@granth23
Copy link
Contributor Author

I’m a bit confused about the second point you mentioned. From my perspective, if a user filters and then links a track, the next search should display only unlinked listens that are still unlinked. Why should the original list be used as the searchable list, considering it might include tracks that have already been linked? Please let me know if I’m misunderstanding something.

@MonkeyDo
Copy link
Member

MonkeyDo commented Jan 28, 2025

I’m a bit confused about the second point you mentioned. From my perspective, if a user filters and then links a track, the next search should display only unlinked listens that are still unlinked. Why should the original list be used as the searchable list, considering it might include tracks that have already been linked? Please let me know if I’m misunderstanding something.

Ah, well precisely! You understand the issue we would encounter, but that is how the current code behaves.
My suggestion is meant to fix this issue.

This line in handleReset in your current code resets the list to the original one passed in props, which is going to contain tracks we might have just linked:
setUnlinkedListens(unlinkedListensProps); // Reset to the original list

To resolve that issue, I think we need to store a modifiable list separately, originally equal to unlinkedListensProps, which we modify as we link listens and then use as the reset value.

EDIT: Another issue is that currently, if you filter results, you can't change the search query and do another search, because the initial filtered list is used as the base for filtering the second time.
Try searching for a word you know is in use for a track, then without resetting, try searching for another word you know is in another track.

@MonkeyDo
Copy link
Member

Deployed to https://test.listenbrainz.org/settings/link-listens/ for further testing

Copy link
Member

@anshg1214 anshg1214 left a comment

Choose a reason for hiding this comment

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

LGTM

@anshg1214 anshg1214 merged commit 1d96131 into metabrainz:master Jan 29, 2025
2 checks passed
@granth23 granth23 deleted the LB-1727 branch January 30, 2025 04:55
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.

3 participants