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-1102, LB-1610, LB-945: Revamp Top Entity stats pages #2937

Merged
merged 38 commits into from
Jul 31, 2024

Conversation

MonkeyDo
Copy link
Member

@MonkeyDo MonkeyDo commented Jul 10, 2024

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:
image
image

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à:

image

The bottom of the graph, the pagination buttons moved up, and the release cards below:
image

Couldn't come up with a better place to put the rank (# 1,etc.) than the title tooltip that appears on hover:
image

On mobile:
image
image

Also fixes LB-945 as I reworked the pagination component as well.

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
@MonkeyDo MonkeyDo requested a review from Aerozol July 10, 2024 16:22
@MonkeyDo
Copy link
Member Author

@Aerozol I haven't deployed this PR anywhere, but the screenshots should do it justice.
I was going for improvement over perfection, so I'm sure we could improve this a lot.
At least the changes here make the page usable, and that was my priority.

@Aerozol
Copy link
Contributor

Aerozol commented Jul 10, 2024

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

@MonkeyDo
Copy link
Member Author

MonkeyDo commented Jul 11, 2024

Thanks for letting me nit-pick your hard work ;P

Thanks for your insight !

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.

It confirms some of the thoughts I had as well, in particular the legibility and alignment of the text in the graph.
I'll see what I can do with text/bars colours.
In particular I chose the two LB colours for the gradient, but it doesn't have to be this way after all. Will look at some of the other gradient colours we use elsewhere.

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)

Gradient is cool though - how does it look left to right? e.g. so the most listened albums are more 'hot'

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.

@MonkeyDo
Copy link
Member Author

MonkeyDo commented Jul 11, 2024

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):

image

Setting the opacity of the bar I'm not convinced by:
image

A bit more fiddling, this might work?
image

@MonkeyDo
Copy link
Member Author

MonkeyDo commented Jul 11, 2024

Gradient is cool though - how does it look left to right? e.g. so the most listened albums are more 'hot'

Do you mean something like this? (I'm learning some advanced SVG gradient techniques, thanks :p )
I mocked up what it would look like with the text left-aligned:
image

And with another gradient from the "created for you page". Is that too MusicBrainz?
image

Or starting lighter and ending with the LB orange but super-saturated?
image

@Aerozol
Copy link
Contributor

Aerozol commented Jul 12, 2024

Looking nice!!

With the gradient, I meant rotating what you already had 90%. I think for sure you can have some more padding around the text?
The bold text, close together, seems quite heavy compared to your light designs for ListenBrainz (e.g. the thin fonts for headers, with a lot of white)

I'm thinking something like this might be more on brand:
image
Potentially even with lighter text.

If the washed-outness is looking too dull maybe we can freshen it up without interfering with the text?
image

@MonkeyDo
Copy link
Member Author

MonkeyDo commented Jul 16, 2024

Alas, we must make some compromises.
Here are the options available to us (please ignore the text for now):

  1. gradient is 'fixed', meaning a shorter bar will show less of the gradient than a long bar:
    Top of the graph:
    image
    Bottom of the graph with less colour:
    image

I managed to get it working for mobile sizes too:
image
image

  1. gradient adapts to the length of each bar:
    It allows us to add that extra little colourful bit at the end (note that I can't vary the colour, it's the same for each bar), but doesn't have that "more listens = warmer color" aspect that we have with solution 1 on desktop
    image
    image

Looks nice on mobile too:
image

@MonkeyDo
Copy link
Member Author

MonkeyDo commented Jul 16, 2024

And the tweaked text colors + artist name added:
image

Artists only:
image

@Aerozol
Copy link
Contributor

Aerozol commented Jul 18, 2024

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 🥇

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
in my bellybutton
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.
also show number of listens for each bar instead of the position (#1, #2, etc.)
@MonkeyDo
Copy link
Member Author

MonkeyDo commented Jul 23, 2024

I pushed a bit more to try and solve the issues I saw, and that other people reported.
In particular, missing were: the listen count for each bar, links to the entities, and a bit more colour (both myself and people I asked thought it looked a bit washed out).

Here is a proposal to resolve these points:
image

The bottom of the graph, the pagination buttons moved up, and the release cards below:
image

Couldn't come up with a better place to put the rank (#1,etc.) than the title tooltip that appears on hover:
image

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.
@MonkeyDo MonkeyDo changed the title LB-1102: Revamp Top Entity stats pages LB-1102, LB-1610, LB-945: Revamp Top Entity stats pages Jul 25, 2024
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.
@MonkeyDo MonkeyDo requested a review from anshg1214 July 25, 2024 16:48
Copy link
Member

@anshg1214 anshg1214 left a 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.

  1. Let's add caching using react query to the API call getData.
  2. 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.

frontend/js/src/user/charts/UserEntityChart.tsx Outdated Show resolved Hide resolved
frontend/js/src/user/charts/UserEntityChart.tsx Outdated Show resolved Hide resolved
frontend/js/src/user/charts/UserEntityChart.tsx Outdated Show resolved Hide resolved
"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.
@MonkeyDo
Copy link
Member Author

MonkeyDo commented Jul 29, 2024

Ran into a bug, hold off on merging please

Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

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

@MonkeyDo MonkeyDo requested a review from anshg1214 July 30, 2024 10:22
Copy link
Member

@anshg1214 anshg1214 left a comment

Choose a reason for hiding this comment

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

LGTM!

@MonkeyDo MonkeyDo merged commit cde6811 into master Jul 31, 2024
2 checks passed
@MonkeyDo MonkeyDo deleted the entity-stats-page branch July 31, 2024 11:25
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.

3 participants