Added avatars and rank summary to user profile cards #7518
Conversation
|
It doesn't let you set null because that's not how it works. You can check existing usages and display which you should share logic with (or at very least follow the same conditional and formatting paths). The formatting should match existing displays everywhere, which it doesn't right now. |
|
By matching formatting are you referring to displaying the stats as something like this?
On the frontend the number formatting is being done with |
|
Seems better, for sure. |
| $globalRankLabel = trans('users.show.rank.global_simple'); | ||
| $globalRankValue = number_format($stats["global_rank"]); | ||
| $countryRankLabel = trans('users.show.rank.country_simple'); | ||
| $countryRankValue = number_format($stats["country_rank"]); |
There was a problem hiding this comment.
this will cause error when the user doesn't have any stats. And this will render 0 (converted from null) if the user isn't ranked (when both either rank_score and or rank_score_index are 0).
| $userData = $jsonChunks["user"]; | ||
| $stats = $userData["statistics"]; | ||
| $globalRankLabel = trans('users.show.rank.global_simple'); | ||
| $globalRankValue = number_format($stats["global_rank"]); |
| 'titlePrepend' => blade_safe(str_replace(' ', ' ', e($user->username))), | ||
| 'pageDescription' => page_description($user->username), | ||
| 'pageDescription' => "{$globalRankLabel}: #{$globalRankValue}\n{$countryRankLabel}: #{$countryRankValue}", |
There was a problem hiding this comment.
it's not actually expected to have newlines in description. For example twitter will render this like so:
| @@ -2,9 +2,24 @@ | |||
| Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the GNU Affero General Public License v3.0. | |||
| See the LICENCE file in the repository root for full licence text. | |||
| --}} | |||
|
|
|||
| @php | |||
| $userData = $jsonChunks["user"]; | |||
| @extends('master', [ | ||
| 'canonicalUrl' => $user->url(), |
There was a problem hiding this comment.
I'm not sure about this one. The canonical url for user page for specific mode (/users/0/mania) is probably the page for specific mode itself (/users/0/mania) not the default one (/users/0).
| @php | ||
| $userData = $jsonChunks["user"]; | ||
| $stats = $userData["statistics"]; | ||
| $globalRankLabel = trans('users.show.rank.global_simple'); |
There was a problem hiding this comment.
the label is missing which mode the rank is for
|
@Xetera are you still interested in applying the review for this pull request? it should be in a good state to get merged if you follow up on that. |
|
Ah sorry, I got a little overwhelmed by the requested changes after I realized the scope of this is bigger than I thought. I'll try to see if I can address some of these over the weekend. Forgot osu had more than 1 game mode kekw |
|
I'm taking over this (and the related ogp issue, I guess 👀 ) |
|
superseded by #10570 since I can't push to this branch |
Before:
After:
I wasn't able to test for cases where the user doesn't have a global rank since mysql doesn't let me set null values on user ranks but I know profiles that don't have global ranks like https://osu.ppy.sh/users/4198271 exist. If that needs to be tested before this is merged I might need a little help with it 🙏.