Yooooomi / your_spotify

Self hosted Spotify tracking dashboard
GNU General Public License v3.0
2.67k stars 109 forks source link

Feature/313 - Add album details view #346

Closed RomainNeup closed 4 months ago

RomainNeup commented 4 months ago

Changelog

Doing the code to handle this feature request : https://github.com/Yooooomi/your_spotify/issues/313

Backend Changes :gear:

Frontend Changes :computer:

image
RomainNeup commented 4 months ago

Hello @Yooooomi could you have a look please :)

quentinguidee commented 4 months ago

cfr. https://github.com/Yooooomi/your_spotify/pull/348#issuecomment-1966614977

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

Yooooomi commented 4 months ago

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 commented 4 months ago

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.

RomainNeup commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 🤯

Yooooomi commented 4 months ago

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 commented 4 months ago

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

RomainNeup commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

Yooooomi commented 4 months ago

I fixed migrations :)

Yooooomi commented 4 months ago

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

Yooooomi commented 4 months ago

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.