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-1737: Create top artists graph showing album details #3170

Merged
merged 26 commits into from
Mar 20, 2025

Conversation

granth23
Copy link
Contributor

Problem

LB-1737: Create top artists graph showing album details

Solution

image

Using the previous data fetch api with some modifications and using nivo.rocks for the same to be displayed as a stacked/segmented bar graph.

Action

  • Let me know if any additional details or corrections are required.
  • If the changes look good, kindly proceed with merging the PR.

@anshg1214 anshg1214 self-requested a review February 13, 2025 13:52
@granth23 granth23 requested a review from anshg1214 February 14, 2025 07:28
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.

The Graph looks good. Apart from the comments, can we change the color scheme of the graph to something using LB Color theme instead?

Also, it'll be great if we could also generate a similar graph for sitewide stats as well.

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.

The graph looks awesome now🔥

I noticed a few improvements which can be made.

@granth23 granth23 requested a review from anshg1214 February 24, 2025 02:56
@MonkeyDo
Copy link
Member

The graph is looking very good!

I think the artist names at the bottom can be tweaked to avoid the overlapping:
image

Maybe using the axisBottom tickRotation, tickPadding or truncateTickAt, or some combination of those?
Ideally keeping as much of it readable, but avoiding this overlapping which doesn't look great with longer names.

It's especially a problem in mobile view with less space available:
image

@granth23 granth23 requested a review from anshg1214 March 18, 2025 19:11
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.

LG!

@anshg1214 anshg1214 merged commit 1832441 into metabrainz:master Mar 20, 2025
3 checks passed
@Maxr1998
Copy link
Contributor

Maxr1998 commented Apr 3, 2025

The new artist activity widget is really cool! But I noticed a small issue with it, it seems that it used the artist name from the recording (listen?), and not the canonical artist name from MusicBrainz to group artists. Thus, I have the same artist twice with different names (one in Latin and one in Hangul).

For what it's worth, my top artists list correctly lists that artist just once.

@MonkeyDo
Copy link
Member

MonkeyDo commented Apr 4, 2025

@Maxr1998 Thanks for reporting the issue!
It's not specifically associated with this new graph, but I think a more general problem with statistics.

Could I ask you to please open a new ticket in https://tickets.metabrainz.org/projects/LB with a screenshot of the stats?

Could you also please check that your top artists for that time range (https://listenbrainz.org/user/Maxr1998/stats/top-artists) whos the same issue, maybe circling or describing the duplicate artist issue?
(This is to ensure that it's a general stats issue rather than linked to the graph)

@Maxr1998
Copy link
Contributor

Maxr1998 commented Apr 5, 2025

Sure, opened a ticket as LB-1778. It appears to be specific to this new graph, though, since my top artists stats are correct.

@granth23 granth23 deleted the LB-1737-User-Artist-Map branch April 6, 2025 09:58
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.

4 participants