-
-
Notifications
You must be signed in to change notification settings - Fork 209
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-1264: Releases by "Various Artists" should not be included on "Fresh releases" page #2688
base: master
Are you sure you want to change the base?
Conversation
This is not the right way to check for various artists. Various artists releases have the special artist_credit_id of 1. So ac.id = 1 in the where clause would work. |
I would suggest doing this filtering on frontend instead so that users have the option to show/hide va releases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preliminary review/questions.
Maybe I'm misunderstanding some aspects here, let me know if I got it all wrong.
Haven't tried running the PR yet, I'm waiting until we merge the search PR to deploy it to test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deployed to test.LB for testing, and working perfectly, nice work !
I do have a question below regarding the odd re-render loop you ran into previously, but feel free to merge if I'm just being thick :P
const releasesString = JSON.stringify(releases); | ||
React.useEffect(() => { | ||
const newReleases = JSON.parse(releasesString); | ||
setFilteredList(newReleases); | ||
}, [releasesString]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to test this locally properly, but I wonder if you need the stringification here.
Does this result int he same re-rendering loop issue still?
If so, I think something smells a bit fishy with the way we manipulate the state between parent and child components...
React.useEffect(() => {
setFilteredList(releases);
}, [releases]);
This PR