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

Add Donation Leaderboard Page #2981

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Add Donation Leaderboard Page #2981

wants to merge 10 commits into from

Conversation

anshg1214
Copy link
Member

@anshg1214 anshg1214 commented Sep 16, 2024

image

@pep8speaks
Copy link

pep8speaks commented Sep 16, 2024

Hello @anshg1214! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-09-18 03:44:40 UTC

Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Super clean!

I love how we can fairly easily set up new pages without having to worry about data loading too much, with all the tools you put in place with the SPA refactor!

Would like some eyes form @Aerozol and @mayhem .
I just redeployed this PR to https://test.listenbrainz.org/donors/

I wonder if it would make sense to have a bit of a paragraph explaining that this is a list of MetaBrainz donors, not specifically ListenBrainz donors. Opinions welcome.

frontend/js/src/components/Navbar.tsx Outdated Show resolved Hide resolved
frontend/css/donors-page.less Outdated Show resolved Hide resolved
frontend/js/src/donors/Donors.tsx Outdated Show resolved Hide resolved
@Aerozol
Copy link
Contributor

Aerozol commented Sep 17, 2024

Is this still on test? What I can see looks good, but it's just the header : D

image

@anshg1214
Copy link
Member Author

@Aerozol Thanks for reporting this issue. This issue has been fixed in #2983

I've also updated the PR description with a screenshot of how the page looks like.

@Aerozol
Copy link
Contributor

Aerozol commented Sep 18, 2024

Looks great! So cool to see top donors!
My feedback:

  1. Double checking that this respects the privacy of anonymous donors in all cases?
  2. Where does the link to the page to donate go? If this page is going live before the donation page we can link to https://metabrainz.org/donors for now
  3. It suppose it technically makes sense to use the 'square' edged tiles/cards (ahh I'm forgetting what we call these in LB) styles, which is what we use in the nearby setting pages. However I think users will be expecting the rounded + shadowed style of the main pages here.
  4. Love the listens stats!
  5. This might need a (?) rollover (or maybe a doc link, if it gets long) that answers questions a lot of users will have (are multiple donations combined into a single count on this page, what currency is this in, does this count metabrainz donations, how do I associate a previous donation with my current/new LB account etc)
  6. This is more of a feature request, but I want to check before making a ticket: I think it would be awesome to show pins here, when any of these users have a current pinned track. I imagine this wouldn't harm the $ donation status. If anyone disagrees please say!

Let me know if there's anything I can help with.
Note that I haven't been able to test the interaction or on mobile, I just looked at the screenshot, so I might have more notes or some of these might be cleared up if I can test.

@@ -98,13 +98,22 @@ function Donors() {
{donor.musicbrainz_id &&
(donor.is_listenbrainz_user ? (
<Link
to={`/user/${donor.musicbrainz_id}`}
to={`${
!donor.is_listenbrainz_user && "https://musicbrainz.org"
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect this condition is never truthy,aas you are already inside a conditional check of donor.is_listenbrainz_user ? (...

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