-
-
Notifications
You must be signed in to change notification settings - Fork 214
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-1102, LB-1610, LB-945: Revamp Top Entity stats pages #2937
Conversation
Put the graph at the top of the page and the list of entities below that. The left/right split that we currently have is unusable on mobile and pretty ugly on desktop, so...
To allow wrapping text especially for narrow mobile phone screens, otherwise the tooltip does not serve its primary purpose on mobile
@Aerozol I haven't deployed this PR anywhere, but the screenshots should do it justice. |
Ooh, very cool!! No notes re. the functionality, but I think the styling needs to be more accessible. In particular the white writing on the orange background is going to give some users difficulty/won't meet accessibility standards. I think we could make the squares larger, giving some space around the text, and left-align the text too. And maybe have the colourful background at 20% or something, and have black text? Or you might find a nicer way to make the text more visible. Gradient is cool though - how does it look left to right? e.g. so the most listened albums are more 'hot' Thanks for letting me nit-pick your hard work ;P |
Thanks for your insight !
It confirms some of the thoughts I had as well, in particular the legibility and alignment of the text in the graph. For the alignment I am waiting for the graph package to be updated. Serendipitously a pull request to allow customizing the alignment of the text was merged just last week! (plouc/nivo#2585)
Not sure I understand what you mean there. The same gradient left to right in each bar? The most listened album is the one on top, I'm not clear on how the gradients would make it look more hot. |
For a simple example to give you an idea, I can generate some complementary/triad/analogous colours based on one of our brand colours, and I can also automatically set the text colour based on readability of the background colour like so (although I might want a better tool, the result is not ideal): |
Looks great! Really! Honestly, I can't pick between 1 & 2 (colourful end part vs having 'cooler' listens at the bottom). I'm happy for you to make the call, I'm really impressed you were able to turn around my figma noodling 🥇 |
if it is in the data
foreignObject allows us to use HTML in the SVG, to use links as well as using CSS to handle text wrapping and ellipses. On mobile try to save some space by moving elements around a bit.
Replaces the ListenCard component for releases and release-groups
We removed the graph tooltips which don't work well with links in the bars , considering we were able to put almost all the information directly in the graph bars. The only missing piece of data is the listen count, so an axis at the top helps read that piece of data. Listens count also available on hover using html title
Calculate the height of the graph based on the number of results, for example for your weekly stats if you don't have 25 artists yet.
I pushed a bit more to try and solve the issues I saw, and that other people reported. Here is a proposal to resolve these points: The bottom of the graph, the pagination buttons moved up, and the release cards below: Couldn't come up with a better place to put the rank (#1,etc.) than the title tooltip that appears on hover: Currently deployed on test.lb https://test.listenbrainz.org/user/mr_monkey/stats/top-artists/?range=week |
currently not sent by the frontend, see LB-1609
We have some releases, artists and recordings that are unmapped, meaning we don't have MBIDs for them. In that case, link to the pre-filled search page rather than linking to e.g. `/artist/undefined`
Tweak the color and size of the bars a bit more, and make the ticks and grid lines for integers only (no sense in showing fractions of a listen...)
Didn't realize we could use the same definition as for the tick value, which is exactly what I wanted.
I wanted to put that information somewhere, as it is impractical to add it to the bar text itself. At least we have somewhere to put that piece of data.
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.
This PR looks good to me. Just some minor fixes.
Further down there can be two improvements in the top-entity stats page.
- Let's add caching using react query to the API call
getData
. - Let's move the pills on the top outside the loader component.
But, let's do these changes in a separate PR.
Also, please update the screenshots in the PR description to the latest version.
"display:grid" with items set to be 400px minimum we causing overflow issues because grid items do not honor min-width. Flexbox to the rescue once again! Also two-line ellipsis for listencard title like we do elsewhere.
In global stats we don't have an entityCount so we should just hide the header + remove useless row + col-xs-12 wrappers which achieve nothing.
Using a .row wrapper with a single .col-xs-12 inside basically does nothing other than making the HTML less readable.
Good old off-by-one error !
Made a mistake previously by setting the number of pages to the max possible number of pages, instead of capping it at that number.
Edit: Try as I might I couldn't replicate it. I think it was probably because I had manipulated nodes in the HTML page directly and React didn't like it when I switched page |
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.
LGTM!
We have long had this side-by-side listencard and graph page, which A. doesn't look great, B. is unusable on mobile and C. the graph doesn't add much as it stands:
Separate the graph and put it at the top where it offers a useful overview while not making the rest of the page unusable.
Keep the listencards below for now, as they are useful for listening to music directly or navigating to the entities' pages.
Use ReleaseCard components for albums instead of ListenCard.
Add a splash of colour to make it less dull, and voilà:
The bottom of the graph, the pagination buttons moved up, and the release cards below:
Couldn't come up with a better place to put the rank (# 1,etc.) than the title tooltip that appears on hover:
On mobile:
Also fixes LB-945 as I reworked the pagination component as well.