Skip to content

Add player profile links to the replay browser #450

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

juebjueb
Copy link

Follow up on PR #375 authored by trstruth.

image

Addressing comments from vinceau:

  1. The inconsistent behaviour of some badges being clickable and some not is quite jarring.
  2. It occupies the badge click functionality in case we wanted to use that behaviour for something else in the future.
  3. The PlayerBadge which should be a dumb component now also has to handle the click function which is undesirable. I'd rather leave that to the parent.
  1. Added an explicit button to eliminate confusion for which badges are clickable.
  2. I agree that a pop-up menu would be nice to allow further extension, but I worry this would be premature. There hasn't been much activity on the replay browser over the last year, and I wouldn't want there to be a menu that's only populated with one item for a long period of time.
  3. Moved from PlayerBadge to its parent TeamElements

@erickaoek
Copy link

This is awesome, someone review please :)

@JLaferri
Copy link
Member

JLaferri commented Apr 10, 2025

I think this is cool and would be great to merge but I think the visual styling still needs a little work. It's not awful but it certainly feels tacked on and like it doesn't fully belong. I'm not sure what my suggestion is to fix it atm though. I'm sure a designer could come up with a clean solution.

@juebjueb
Copy link
Author

Probably the visual issue is that the links are in between the player badges and the "vs". How about moving to the replay browser like so? Adds an extra click to get there, but looks nicer.

image

@JLaferri
Copy link
Member

Probably the visual issue is that the links are in between the player badges and the "vs". How about moving to the replay browser like so? Adds an extra click to get there, but looks nicer.

I think I would personally be okay with that for now.

@juebjueb
Copy link
Author

Great, thanks! Ready for review.

@vinceau
Copy link
Member

vinceau commented Apr 20, 2025

what about moving the link to the actual connect code at the top of the replay stats page banner?

@juebjueb
Copy link
Author

We could, but this would have the same visual jank as the before where the links are in between the player names and the VS which breaks symmetry

@vinceau
Copy link
Member

vinceau commented Apr 20, 2025

We could, but this would have the same visual jank as the before where the links are in between the player names and the VS which breaks symmetry

by links and visual symmetry are you referring to the external link visual indicator? because i was imagining not having that visual indicator and just having like an underline or something to indicate interactivity and clickable link. i don't see how that would break symmetry or have visual jank

@JLaferri
Copy link
Member

I think the tradeoff there is that it's a little less discoverable, but maybe it makes more sense to have the link tied to the connect code?

Honestly I'm okay with either. If it's on the connect code I don't really like underlining it. But a hover state with a hand and font color change (maybe a darker gray or something) is fine. You could look at how the hover state of the "Support Slippi" link works in the bottom right of the app.

@juebjueb
Copy link
Author

Done, see latest.

image

`}
href={slippiProfileUrl}
>
<div>{connectCode}</div>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<div>{connectCode}</div>
{connectCode}

can you remove the nested div here? we shouldn't need it.

@juebjueb
Copy link
Author

done

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.

4 participants