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

Fix leaderboard order #2988

Merged
merged 1 commit into from
Jun 10, 2023
Merged

Fix leaderboard order #2988

merged 1 commit into from
Jun 10, 2023

Conversation

BlackYps
Copy link
Collaborator

@BlackYps BlackYps commented Jun 9, 2023

The 3v3 was listed last, because it has the highest id. So we have to switch to a different way to sort them, so they are in the order that you would expect.
grafik

@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Merging #2988 (783590b) into develop (329c775) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 783590b differs from pull request most recent head 37ca19b. Consider uploading reports for the commit 37ca19b to get more accurate results

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #2988      +/-   ##
=============================================
- Coverage      61.33%   61.33%   -0.01%     
  Complexity      4594     4594              
=============================================
  Files            557      557              
  Lines          20185    20185              
  Branches        1048     1048              
=============================================
- Hits           12381    12380       -1     
  Misses          7218     7218              
- Partials         586      587       +1     
Impacted Files Coverage Δ
...ver/client/leaderboard/LeaderboardsController.java 86.36% <100.00%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 329c775...37ca19b. Read the comment docs.

@BlackYps BlackYps requested a review from Sheikah45 June 10, 2023 13:58
@Sheikah45
Copy link
Member

So we just switch to sorting by the name? Wouldn't this potentially have the same issue in the future? Maybe we should add an order parameter to the league object

@BlackYps
Copy link
Collaborator Author

I agree in principle, but adding an order parameter would be more than one order of magnitude more work. It would require changes to the league service, the commons repo and finally the client. All of this to fix a problem that we might have in the future. At the moment I don't know of any plans to add more leagues.
So I propose that for now we roll with this change and if we notice that it actually causes problems in the future, then we can implement the more elaborate solution. Adding a new league would require changes to the league service anyway, so I could then easily add a new order parameter as well.

@BlackYps BlackYps force-pushed the leaderboard-order branch from 493ef56 to 37ca19b Compare June 10, 2023 19:53
@Sheikah45 Sheikah45 merged commit 5f65e3a into develop Jun 10, 2023
@Sheikah45 Sheikah45 deleted the leaderboard-order branch June 10, 2023 22:15
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.

2 participants