Skip to content

Conversation

@Mehdi9120310391
Copy link
Contributor

feat(Infobox): add positions parameter for Rematch

Summary

For the introduction of the Rematch wiki, I modified the Infobox to ensure that players' "positions" can be listed in their Infobox.

How did you test this change?

On my page: https://liquipedia.net/rematch/Eraclesss

Dev: mej

feat(Infobox): add positions parameter for Rematch
@hjpalpha hjpalpha added the g: rematch REMATCH label Jul 8, 2025
@hjpalpha hjpalpha changed the title Create Custom.lua feat(infobox): Add rematch Infobox player Jul 8, 2025
@Mehdi9120310391
Copy link
Contributor Author

all requested changes have been applied!

Comment on lines 62 to 106
---@param args table
---@param birthDisplay string
---@param personType string
---@param status PlayerStatus
---@return string[]
function CustomPlayer:getCategories(args, birthDisplay, personType, status)
if not Namespace.isMain() then return {} end

local categories = {}
local roles = String.isNotEmpty(args.roles) and Array.parseCommaSeparatedString(args.roles, ',') or {}

Array.forEach(roles, function(role)
local roleCategory = role .. 's'
table.insert(categories, roleCategory)
end)

Array.forEach(self:getLocations(), function(country)
local demonym = Flags.getLocalisation(country)
if demonym then
Array.forEach(roles, function(role)
local roleCategory = demonym .. ' ' .. role .. 's'
table.insert(categories, roleCategory)
end)
end
end)

if String.isNotEmpty(args.positions) then
local positions = Array.parseCommaSeparatedString(args.positions, ',')
local validPositions = Array.filter(positions, function(pos)
return String.isNotEmpty(pos)
end)
Array.forEach(validPositions, function(pos)
local category = String.upperCaseFirst(pos) .. 's'
table.insert(categories, category)
Array.forEach(self:getLocations(), function(country)
local demonym = Flags.getLocalisation(country)
if demonym then
table.insert(categories, demonym .. ' ' .. category)
end
end)
end)
end

return categories
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

heavily opposed to this
use CustomPlayer:getWikiCategories(categories) instead to add wiki specific categories!
(although on a new wiki i very highly doubt it is necessary at all)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the categories for positions with nationality, but I kept the categories for the positions themselves.

Copy link
Collaborator

Choose a reason for hiding this comment

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

which still doesn't catch the biggest issue of the comment
you are overwriting the entire category handling from commons, which is a no-go

@Rathoz
Copy link
Collaborator

Rathoz commented Jul 8, 2025

Use the default roles input over the custom position input

@Mehdi9120310391
Copy link
Contributor Author

Mehdi9120310391 commented Jul 8, 2025

Use the default roles input over the custom position input

that's what I wanted to do at first, but given all the possibilities, I thought it would be good to leave the choice open.

@Mehdi9120310391
Copy link
Contributor Author

I think the file is ready to be merged, if everyone is okay?

@hjpalpha
Copy link
Collaborator

hjpalpha commented Jul 9, 2025

I think the file is ready to be merged, if everyone is okay?

NO!

  • there are still unresolved comments
  • the comment from rath (and me agreeing with it) that pos should likely be nuked in fav of using commons role handling is still up for discussion (likely we follow what rath suggests)
  • rathos (the one that merges) is on vacation

@Mehdi9120310391
Copy link
Contributor Author

Mehdi9120310391 commented Jul 9, 2025

I think the file is ready to be merged, if everyone is okay?

NO!

  • there are still unresolved comments
  • the comment from rath (and me agreeing with it) that pos should likely be nuked in fav of using commons role handling is still up for discussion (likely we follow what rath suggests)
  • rathos (the one that merges) is on vacation

About positions, I would like to add default ones, the problem is that I don't know all of them because players can be Mid/Mid Offensive/Central Forward and lot more, but if I can suggest something is to let it on custom param from now on, and change it after when we'll have all positions known generally, but if you prefer some ones now, I'll work on it

@Mehdi9120310391
Copy link
Contributor Author

Any update?

Copy link
Collaborator

@hjpalpha hjpalpha left a comment

Choose a reason for hiding this comment

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

as already mentioned before not a fan of the position stuff
and since rath is opposed to it too i deem it unlikely to get approval

---@param personType string
---@param status PlayerStatus
---@return string[]
function CustomPlayer:getCategories(args, birthDisplay, personType, status)
Copy link
Collaborator

Choose a reason for hiding this comment

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

as already mentioned before
do not overwrite the commons function!
if you actually need to append the list of categories with wiki specific ones use CustomPlayer:getWikiCategories(categories) instead!

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