Skip to content

feat(TeamParticipants): ommit ingame role display for roles without icon#7523

Merged
Eetwalt merged 9 commits into
mainfrom
7494-alternative
May 19, 2026
Merged

feat(TeamParticipants): ommit ingame role display for roles without icon#7523
Eetwalt merged 9 commits into
mainfrom
7494-alternative

Conversation

@hjpalpha
Copy link
Copy Markdown
Collaborator

@hjpalpha hjpalpha commented May 15, 2026

depends on #7529

Summary

  • only display ingame roles if they have an icon
  • for non ingame roles display icon if available, use current display if no icon is available
  • fix a missing anno

How did you test this change?

dev

@hjpalpha hjpalpha changed the title Update Roster.lua feat(TeamParticipants): ommit ingame role display for roles without icon May 15, 2026
@hjpalpha hjpalpha marked this pull request as ready for review May 15, 2026 19:37
@hjpalpha hjpalpha requested review from a team as code owners May 15, 2026 19:37
@hjpalpha hjpalpha mentioned this pull request May 15, 2026
42 tasks
@hjpalpha
Copy link
Copy Markdown
Collaborator Author

if this is liked i can go through the ingame roles and add the iconFa for igl/captain where those exist

@Rathoz
Copy link
Copy Markdown
Collaborator

Rathoz commented May 16, 2026

Medium term captain (and similiar) should not be in ingameroles. Ingame roles should only be actual game based positions. So based on current plans this adds tech debt for a small short-term gain

@mbergen
Copy link
Copy Markdown
Collaborator

mbergen commented May 16, 2026

Medium term captain (and similiar) should not be in ingameroles. Ingame roles should only be actual game based positions. So based on current plans this adds tech debt for a small short-term gain

That's what i suggested in the linked issue (#7491). In which set of roles would captain belong, a new one?

@hjpalpha
Copy link
Copy Markdown
Collaborator Author

hjpalpha commented May 16, 2026

Medium term captain (and similiar) should not be in ingameroles. Ingame roles should only be actual game based positions. So based on current plans this adds tech debt for a small short-term gain

the only debt we create with this pr is adding the ingame roles modules for sc, sc2, sg
the rest is independent of captain being an ingame role or not

i am okay with creating a fourth role type for it, but don't have a good name for it
(fwiw teamParticipants would need some small adjusts for that if we want to display them as icon)

adding captain to staff roles or contract roles is way way way worse than having them in ingame roles
it would break quite some stuff and also fuck up teamParticipants storage for anyone with those roles (would be stored as staff instead of player which is just completely wrong)

@Eetwalt
Copy link
Copy Markdown
Collaborator

Eetwalt commented May 18, 2026

i am okay with creating a fourth role type for it, but don't have a good name for it
(fwiw teamParticipants would need some small adjusts for that if we want to display them as icon)

I think in this PR we can just leave the parts in which don't display the text on the left, only icons.
And make another one suggesting a fourth role category. Name candidates for this category could be PlayerRole as salt suggested, TeamRole or even PlayerTeamRole ?

@hjpalpha
Copy link
Copy Markdown
Collaborator Author

hjpalpha commented May 18, 2026

i am okay with creating a fourth role type for it, but don't have a good name for it
(fwiw teamParticipants would need some small adjusts for that if we want to display them as icon)

I think in this PR we can just leave the parts in which don't display the text on the left, only icons. And make another one suggesting a fourth role category. Name candidates for this category could be PlayerRole as salt suggested, TeamRole or even PlayerTeamRole ?

hmm,
will split it up into 3 PRs when i find the time

Comment thread lua/wikis/commons/Widget/Participants/Team/Roster.lua Outdated
@hjpalpha hjpalpha requested a review from Eetwalt May 19, 2026 09:56
@Eetwalt Eetwalt merged commit e5a4e3e into main May 19, 2026
8 checks passed
@Eetwalt Eetwalt deleted the 7494-alternative branch May 19, 2026 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants