Skip to content

Conversation

trichoplax
Copy link
Contributor

Fixes Missing usernames on leaderboard which I've only just posted - it doesn't have a GitHub issue yet.

We recently made the user avatar in the user card under a post into a link to the user profile page. This means there are now 2 elements with the class .user-card--link per user card, and the leaderboard code was trying to find a username in the avatar instead of the text link.

This pull request makes the selector more specific to avoid this problem.

I'm not on my development machine so I've only tested this in the browser developer window on the live site, not in development. Making a pull request now to remind me to test this when I can, and to allow anyone else to test if you wish.

@Oaphi Oaphi requested a review from a team May 1, 2025 00:34
Copy link

codecov bot commented May 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.55%. Comparing base (7e9dd90) to head (5974378).
Report is 4 commits behind head on develop.

Additional details and impacted files
Components Coverage Δ
controllers 60.48% <ø> (ø)
helpers 68.84% <ø> (ø)
jobs 28.00% <ø> (ø)
models 81.87% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@Oaphi Oaphi left a comment

Choose a reason for hiding this comment

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

LGTM, tested locally and made a couple of improvements on top:

  1. Added some JSDoc for type checking in TS-aware IDEs (barebones, feel free to expand upon);
  2. Ensured getLeaderboard doesn't cause a runtime error if header is not found in some of the entries;
  3. Added a bit of padding to the right of the now restrored usernames;

@Oaphi Oaphi added area: frontend Changes to front-end code area: assets Changes to public assets labels May 1, 2025
@Oaphi Oaphi self-assigned this May 1, 2025
@cellio
Copy link
Member

cellio commented May 1, 2025

I apologize. I made the change that caused this and it never occurred to me that there was a dependency in the leaderboard.

@Oaphi
Copy link
Member

Oaphi commented May 1, 2025

@cellio that's fine, the issue is more that we neither provide a hook there for scripts to use (in other places, we mark elements that are used by our JS assets with classes prefixed with js-* [which is common practice]) nor do we have an API yet to avoid coupling to specific markup structure.

@trichoplax
Copy link
Contributor Author

@cellio echoing @Oaphi there's nothing to apologise for. Every change is seen by at least 2 people (commiter and reviewer) and I wouldn't expect either of them to spot something that no amount of manual testing can find without going through every existing community (please don't start doing that!).

I've raised #1609 to discuss ways of automatically testing the communities that have custom JS. On top of that, as pointed out, once we have an API this sensitivity of the leaderboard to HTML changes will go away.

And if you insist on apologising, then I'm sorry too - I both requested the change and looked over the pull request before it was merged and I didn't spot this either.

@trichoplax
Copy link
Contributor Author

@Oaphi thanks for the extra improvements - those all look good too. Does this mean we need a third person to review now...?

@Oaphi Oaphi merged commit c44eae6 into develop May 2, 2025
9 checks passed
@Oaphi Oaphi deleted the trichoplax/fix-missing-username-on-code-golf-leaderboard branch May 2, 2025 17:26
@Oaphi
Copy link
Member

Oaphi commented May 2, 2025

@Oaphi thanks for the extra improvements - those all look good too. Does this mean we need a third person to review now...?

No need to, I've merged the PR - a cross-review we've done is enough in this case, imo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: assets Changes to public assets area: frontend Changes to front-end code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants