-
Notifications
You must be signed in to change notification settings - Fork 249
Add playlist info #631
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
Add playlist info #631
Conversation
Hey @jacksongoode could you check this out #630 should get merged before this one. This is the start to being able to add the ability to view users |
@jacksongoode any chance to have a quick look at this? |
Very cool! A few thoughts.
Could you rebase on main? |
Yes that makes sense, I agree. I will have time to do this in a few days! |
Should look better now! |
I'll have a look tonight! It would also be great if you added a short description on the PR just to get a sense for historical record of what the PR does. |
Done and thanks! |
Okay, that makes sense - I agree. I'm sure I can find an endpoint that provides the playlist creation date! Personally, I don't think the cover would look good if it's too far to the right. For playlists, it aligns better with the text or the album covers. Also, unlike albums, playlists don't have track numbers. That said, I'm happy to implement it your way if you'd prefer! I agree with keeping the design almost the same as the albums, except for that one difference. |
Would you be up for me making a few changes and we can revert if you don't think they work? |
Of course please do! |
I was trying this out and realized that maybe the collaborative label isn't accurate? Its always false for my playlists, even collaborative ones? Does it work for you? |
Oh! I don't have any collaborative ones so I couldn't check! |
![]() I made some changes to adapt a bit the last line will show if its public/private and if its collaborative that's added like "Public • Collaborative". @SO9010 what do you think? If you test & think its good feel free to merge! |
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.
See comment above ^
Looks much better, thank you! One thing though I am thinking some hierarchy to the text like the one for albums would look nice. It currently looks a bit flat if that makes sense. I will change that then ask you to check it out again :) |
I think this look better! @jacksongoode just waiting for you final ok again now :) |
Looks good! Merge away! |
This PR aims to add the playlist image, description and owner to the top of the playlist, much like on the top of Albums and Artists. This will be useful when users have been implemented so we can allow the user to navigate to other users by clicking on the username.