Add highest rank tooltip to global rank display #27107
Conversation
Joehuu
commented
Feb 10, 2024
Joehuu
commented
| this PR | Web |
|---|---|
 |
 |
| @@ -34,6 +34,19 @@ public class APIUser : IEquatable<APIUser>, IUser | |||
| [JsonProperty(@"previous_usernames")] | |||
| public string[] PreviousUsernames; | |||
|
|
|||
| [JsonProperty(@"rank_highest")] | |||
| [CanBeNull] | |||
| public UserRankHighest RankHighest; | |||
There was a problem hiding this comment.
and this class keeps growing and growing and growing and growing.........
| Content = s.NewValue?.GlobalRank?.ToLocalisableString("\\##,##0") ?? (LocalisableString)"-"; | ||
| }, true); | ||
|
|
||
| // needed as statistics doesn't populate User |
There was a problem hiding this comment.
Can you elaborate? I don't follow what this is trying to say.
There was a problem hiding this comment.
Talking about this:
osu/osu.Game/Users/UserStatistics.cs
Lines 17 to 18 in b0420f7
Seems to return null, so I have to add the User bindable instead of just using UserStatistics.
There was a problem hiding this comment.
Again, this is what happens when API models are overloaded to be used in 30 different contexts.........
I'd at least reword the comment to mention the specific class rather than vaguely mention "statistics" of unknown provenance.
osu.Game/Users/UserRankPanel.cs
Outdated
| UserStatistics = { BindTarget = statistics }, | ||
| // TODO: make highest rank update, as api.LocalUser doesn't update | ||
| // maybe move to statistics in api, so `SoloStatisticsWatcher` can update the value | ||
| User = { BindTarget = user }, |
There was a problem hiding this comment.
I'm not sure how changes in here are going to interact with #27128. I'm guessing that they are not going to do so at all, since I don't believe anything refetches user on ruleset change (and that's probably a good thing).
Feels like too many cooks in the area. Maybe best to back these changes out for now. Or else wait for that pull to go through.
|
Are you willing to back out the |
|
Yeah will do, wasn't worth centralizing when the |
Also don't show on `LoginOverlay` usage for now.
