Skip to content

feat: Make player tags in the replay browser clickable #375

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 1 commit into
base: main
Choose a base branch
from

Conversation

trstruth
Copy link

@trstruth trstruth commented Mar 8, 2023

Hello! I implemented a tiny feature whereby you can click a players name on the replay browser and it will open their profile on slippi.gg in your browser:

out

I picked this up after I saw it was a feature request in the discord channel

A couple things to note:

  • not all tags are clickable, as not all replays are generated online (this is shown briefly in the above gif). We rely on both prefix and suffix in const [prefix, suffix] = text.split("#"); being non-null make a tag clickable. Essentially the click handler can undefined, which implicitly handles this case. If we think this is ugly I can try to make it more explicit.
  • The fact that InternalPlayerBadge doesn't render the tags led me to pass the click handler into it, but perhaps we should consider consolidating all of the logic into one component - not sure if there's some other prior motivation for separating InternalPlayerBadge from the exported PlayerBadge
  • I didn't add any tests, couldn't find any examples in the code base of existing tests and was wondering if they exist atm

This is my first time contributing to this project, so I did my best to follow examples of other PRs I saw in the repo but please let me know if I'm not properly following any conventions here

@vinceau
Copy link
Member

vinceau commented Mar 9, 2023

Hey, thanks for this PR! I think this is something we'd like to add in the future but this current implementation is not it. Here are some reasons why:

  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.

You're welcome to try address some of these issues if you find yourself with time but both me and Nikki are very busy and may find it hard to give you sufficient feedback to get the PR in a state that is mergable.

Here are some suggestions for how I would see this feature implemented:

  • Leave the player badge as a dumb component, and handle the on click in the parent
  • On click, show a menu (like the overflow button) which gives you an option to Show player profile if it's a valid connect code. Maybe this option can be grayed out/disabled if it's not a connect code. This fixes 1 and 2 and since it's a menu, it also allows us to show more options in the future.

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