Add user card with global/country ranks to login overlay #26340
Conversation
|
Code quality failing |
osu.Game/Users/UserRankPanel.cs
Outdated
| new ProfileValueDisplay(true) | ||
| { | ||
| Title = UsersStrings.ShowRankGlobalSimple, | ||
| Content = User.Statistics?.GlobalRank?.ToLocalisableString("\\##,##0") ?? (LocalisableString)"-" | ||
| }, | ||
| new ProfileValueDisplay(true) | ||
| { | ||
| Title = UsersStrings.ShowRankCountrySimple, | ||
| Content = User.Statistics?.CountryRank?.ToLocalisableString("\\##,##0") ?? (LocalisableString)"-" | ||
| } |
There was a problem hiding this comment.
I dunno. These are never going to update. I'd say they need to update as a minimum requirement to have this added, else it's misleading to the user.
There was a problem hiding this comment.
Sure, I'll look into it
There was a problem hiding this comment.
There probably needs to be a global event which is triggered by the results screen when a change to statistics happens, which can schedule a future update of this card (next time the panel is opened).
Probably out of scope for this PR, but I'd hope to have that as a prereq or follow-up PR so they can be merged together.
There was a problem hiding this comment.
Could updating APIAccess.LocalUser bindable (and binding the card updates to it) be an option here? To me it feels like updating all of the user info might be better than just statistics
There was a problem hiding this comment.
You're probably definitely going to run into the fact that APIUser equality will cause a no-op when you update that bindable.
Maybe separating Statistics into a separate bindable on APIAcess if that is enough to make things work? Then having results screen trigger a apiAccess.RefreshLocalUser() or something, which updates the bindable statistics.
| @@ -110,6 +107,22 @@ private void load() | |||
| Text = User.Username, | |||
| }; | |||
|
|
|||
| protected virtual Drawable? CreateBackground() => Background = new UserCoverBackground | |||
There was a problem hiding this comment.
This is so weird. So there's one method called CreateX and it's virtual and requires override to do something, then there's three other Create methods which are required to be called by the implementation to actually do something.
At very least this needs xmldoc. Optimally, they shouldn't use the same naming structure.
osu.Game/Users/UserRankPanel.cs
Outdated
| { | ||
| Empty(), | ||
| Empty(), | ||
| Empty(), | ||
| Empty() |
There was a problem hiding this comment.
Padding, similar to how UserGridPanel does it
There was a problem hiding this comment.
Please add an inline comment explaining this.
osu.Game/Users/UserRankPanel.cs
Outdated
| details = new FillFlowContainer | ||
| { | ||
| AutoSizeAxes = Axes.Both, | ||
| Direction = FillDirection.Horizontal, | ||
| Spacing = new Vector2(6), | ||
| Children = new Drawable[] | ||
| { | ||
| CreateFlag(), | ||
| } | ||
| } |
There was a problem hiding this comment.
i'm hoping this was just bad copy and paste and you haven't used an AI to write this.
There was a problem hiding this comment.
All of the top layer of the card was pretty much one to one copied from the UserGridPanel. If you're wondering why its a flow - its for the supporter icon which is being added later
There was a problem hiding this comment.
Can you add inline comments for places where code doesn't explain itself?
There was a problem hiding this comment.
Sure, should I do the same in the UserGridCard as well?
|
Okay so I went to repair this but it turns out you've opened a can of worms. Specifically that |
|
Yep I wasn't pushing changes here until #26356 merge, will fix stuff up now. As for the |
| @@ -139,6 +139,8 @@ private void load() | |||
| }, | |||
| }; | |||
|
|
|||
| api.LocalUser.Value.Status.BindValueChanged(e => updateDropdownCurrent(e.NewValue), true); | |||
There was a problem hiding this comment.
i'll fix this, but note that as you didn't create a local bindable this could cause a bindable leak. only saved by the fact there's only ever one LoginPanel.
There was a problem hiding this comment.
gotcha, will keep in mind
osu.Game/Users/UserRankPanel.cs
Outdated
| { | ||
| BorderColour = ColourProvider?.Light1 ?? Colours.GreyVioletLighter; | ||
|
|
||
| api.Statistics.ValueChanged += e => |
There was a problem hiding this comment.
same deal, incorrect bindable usage
Part of #22130, closes #2510
Statisticsbindable toIAPIProviderand update it fromSoloStatisticsWatcher#26356I'm aware that unexpected PRs like this are not really welcomed, but I believe lack of quick way to see your rank is something that people will complain a lot about after pp release, though you can throw it into the backlog if you think it's not that important.
Design is a combination of current user card with expanded user card view from figma. I think it's good enough as a temporary solution and proper design can be implemented whenever design for the new user cards gets finalized.