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-1264: Releases by "Various Artists" should not be included on "Fresh releases" page #2688

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

anshg1214
Copy link
Member

@anshg1214 anshg1214 commented Dec 28, 2023

This PR

  1. Adds filtering option for Various Artists in the frontend
  2. Caches the API calls for fresh release page

@amCap1712
Copy link
Member

amCap1712 commented Dec 28, 2023

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.

@amCap1712
Copy link
Member

I would suggest doing this filtering on frontend instead so that users have the option to show/hide va releases.

@anshg1214 anshg1214 requested a review from MonkeyDo May 30, 2024 12:50
Copy link
Contributor

@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.

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

frontend/js/src/explore/fresh-releases/FreshReleases.tsx Outdated Show resolved Hide resolved
frontend/js/src/explore/fresh-releases/FreshReleases.tsx Outdated Show resolved Hide resolved
frontend/js/src/explore/fresh-releases/FreshReleases.tsx Outdated Show resolved Hide resolved
frontend/js/src/explore/fresh-releases/FreshReleases.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

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

Comment on lines +276 to +280
const releasesString = JSON.stringify(releases);
React.useEffect(() => {
const newReleases = JSON.parse(releasesString);
setFilteredList(newReleases);
}, [releasesString]);
Copy link
Contributor

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]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants