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

Feature/313 - Add album details view #346

Merged
merged 11 commits into from
Feb 28, 2024

Conversation

RomainNeup
Copy link

@RomainNeup RomainNeup commented Feb 21, 2024

Changelog

Doing the code to handle this feature request : #313

Backend Changes ⚙️

  • Added a new endpoint to retrieve album information:

    • The endpoint now returns album details, including the first and last track listening date, tracks, and the artists.
  • Introduced a new endpoint to retrieve album rankings:

    • The endpoint now returns information on 3 albums: one ranked better, the current one, and the one immediately after it in the ranking.
  • Updated the endpoint for retrieving artist statistics:

    • Added functionality to retrieve the Most Listened Album of the Artist.

Frontend Changes 💻

  • Implemented a new album detail page:

    • Includes artist context along with tracks listened.
    • Displays album listened tracks.
    • Shows the first and last time each track was listened to.
  • Added links to album pages wherever an album is mentioned across all pages.

  • Added a most listened album panel to the artist page.

image

@RomainNeup
Copy link
Author

Hello @Yooooomi could you have a look please :)

@quentinguidee
Copy link
Contributor

cfr. #348 (comment)

I think that all requests using InfosModel can use the performance boost from #348, however requests in this PR seem good for now since they are taking the same time than all other requests. I will probably try to refactor all requests at once after this PR is merged.

For the rest of this PR, the result seems pretty good! However I think there are some ESLint errors that need to be fixed before merging

Comment on lines 18 to 20
<Link to={`/album/${album.id}`} className={s.root}>
{album.name}
</Link>
Copy link
Owner

Choose a reason for hiding this comment

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

Isn't that what InlineAlbum does?

Copy link
Author

Choose a reason for hiding this comment

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

We are in InlineAlbum, and it wasn't 🤔 Maybe I didn't got your comment

Comment on lines 95 to 97
onArtistClick={goToArtist}
onTrackClick={goToTrack}
onAlbumClick={goToAlbum}
Copy link
Owner

Choose a reason for hiding this comment

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

Nitpick, but would be nicer to have onTrackClick, onAlbumClick, onArtistClick in this order, from small container to bigger

Copy link
Author

Choose a reason for hiding this comment

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

Done, but anyway we should get rid of this and find a cleaner way to do it. It doesn't seem very maintainable.

BTW the order (artist > track > album) is following the actual order in the search result popup.

Comment on lines 20 to 22
onArtistClick,
onTrackClick,
onAlbumClick,
Copy link
Owner

Choose a reason for hiding this comment

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

same here

return (
<FullscreenCentered>
<Text element="h3">
You never listened to this song, might be someone else registered
Copy link
Owner

Choose a reason for hiding this comment

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

You never listened to this Album

Comment on lines 171 to 176
{ $lookup: {
from: 'tracks',
localField: 'id',
foreignField: 'id',
as: 'track'
}},
Copy link
Owner

Choose a reason for hiding this comment

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

yes indeed is the same pattern as #348

Copy link
Author

Choose a reason for hiding this comment

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

I'll let @quentinguidee do the edit in his MR after me merged this one has mentioned in this comment : #346 (comment)

@Yooooomi
Copy link
Owner

I see you have the total time you listened to a specific album. Would be nice, for consistency, to have the same stat in the artist and track stat page (the times listened and total duration underneath). Nice design btw I like it :)

@Yooooomi
Copy link
Owner

Yooooomi commented Feb 27, 2024

Hello again. I realised I forgot to thank you for the job you've done. Thanks a lot man it means a lot that people get into the project to add new features!
I just pushed a new file architecture to the release 1.8.0 branch. You might want to merge this branch into yours since this merge request is going to target the 1.8.0 release branch (release/1.8.0).

EDIT: just changed the target branch for this PR, I am sorry seeing all the merge conflicts.

@Yooooomi Yooooomi changed the base branch from master to release/1.8.0 February 27, 2024 22:56
@RomainNeup
Copy link
Author

I see you have the total time you listened to a specific album. Would be nice, for consistency, to have the same stat in the artist and track stat page (the times listened and total duration underneath). Nice design btw I like it :)

I will do it in another MR if you don't mind, as it has nothing to do with the topic of this one :)

@Yooooomi
Copy link
Owner

Yooooomi commented Feb 28, 2024

I kinda don't agree as consistency should be done as we develop new features so we don't pile up inconsistency debt.

But don't worry I'll take care of this before releasing.

@RomainNeup
Copy link
Author

I kinda don't agree as consistency should be done as we develop new features so we don't pile up inconsistency debt.

But don't worry I'll take care of this before releasing.

I mean, I did it already but in another branch. So don't worry, I will just include it in this PR

@RomainNeup
Copy link
Author

cfr. #348 (comment)

I think that all requests using InfosModel can use the performance boost from #348, however requests in this PR seem good for now since they are taking the same time than all other requests. I will probably try to refactor all requests at once after this PR is merged.

For the rest of this PR, the result seems pretty good! However I think there are some ESLint errors that need to be fixed before merging

Thanks 🙏
For the ES lint errors, I don't have any on my side 🤔 Do you have a log you could provide or something ?

@quentinguidee
Copy link
Contributor

quentinguidee commented Feb 28, 2024

I got them when starting the dev.sh containers yesterday. However that was before the recent changes in release/1.8.0, so maybe they are not present anymore. Also I think there are also issues with eslint in release/1.8.0 so you can probably ignore them for now

@Yooooomi
Copy link
Owner

Yooooomi commented Feb 28, 2024

Yeah eslint in 1.8.0 is a mess, I've set up a stricter linting than before so don't bother about it! Thanks a lot guys for the work. I'll try to merge them by tomorrow evening or so. You guys rock

@RomainNeup
Copy link
Author

Yeah eslint in 1.8.0 is a mess, I've set up a stricter linting than before so don't bother about it! Thanks a lot guys for the work. I'll try to merge them by tomorrow evening or so. You guys rock

I'm still trying to merge the branch into my branch. What a nightmare 🤯

Comment on lines 20 to 24
onArtistClick,
export default function SideSearch({
onTrackClick,
onArtistClick,
onAlbumClick,
inputClassname,
}: ArtistSearchProps) {
}: SideSearchProps) {
Copy link
Owner

Choose a reason for hiding this comment

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

Typo, Sider

@Yooooomi
Copy link
Owner

Yooooomi commented Feb 28, 2024

I'm still trying to merge the branch into my branch. What a nightmare 🤯

I can take care of it if you want. Basically everything in client/ is now in apps/client

@RomainNeup
Copy link
Author

Should be good but if you can check if I didn't break anything you made. Could be nice thanks 🙏

@RomainNeup
Copy link
Author

Since the merge, I'm getting a Error: No private data found, cannot sign JWT 2 do you know where it does come from ?

@Yooooomi
Copy link
Owner

Since the merge, I'm getting a Error: No private data found, cannot sign JWT 2 do you know where it does come from ?

It seems that you did not run migrations. Do you run the project using docker and dev.sh?

@Yooooomi
Copy link
Owner

I am also having issues with migrations. I'll keep in touch.

@Yooooomi
Copy link
Owner

I fixed migrations :)

@Yooooomi Yooooomi merged commit 8e9bf0d into Yooooomi:release/1.8.0 Feb 28, 2024
@Yooooomi
Copy link
Owner

Yooooomi commented Feb 28, 2024

I'm getting a blank page for album page, I'm trying to fix.
EDIT: It was just the Route missing.

@Yooooomi
Copy link
Owner

Everything works! Nice job, thanks for everything :)
Everything is ready for release 1.8.0. If you think we should wait for the time icon design before publishing it's ok otherwise we can go like this.

@RomainNeup RomainNeup deleted the feature-313 branch March 1, 2024 11:36
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.

None yet

4 participants