Skip to content
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

feat(match2): clash royale support #4321

Merged
merged 35 commits into from
Sep 19, 2024
Merged

feat(match2): clash royale support #4321

merged 35 commits into from
Sep 19, 2024

Conversation

FenrirWulf
Copy link
Collaborator

@FenrirWulf FenrirWulf commented Jun 7, 2024

Summary

Add CR MGI

How did you test this change?

dev (there is a non git version running on the wiki currently ...)

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.

linter is complaining quite a bit too

@FenrirWulf
Copy link
Collaborator Author

linter is complaining quite a bit too

Yeah.. im working through it now. The MatchGroup Input is going to be a pain to rewrite. Plus missing all annotations

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.

the input module is far of from standard
you can make use of several commons functions for sure
additionally it needs lots of cleanup

and annotations missing entirely

only skimmed through the first 150 lines (am on mobile)

@FenrirWulf FenrirWulf requested a review from hjpalpha June 30, 2024 23:48
@FenrirWulf FenrirWulf marked this pull request as ready for review June 30, 2024 23:48
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.

additionally check if Module:CharacterIcon can be used in the display (MatchSummary)

@FenrirWulf
Copy link
Collaborator Author

additionally check if Module:CharacterIcon can be used in the display (MatchSummary)

isnt it already being imported on line 3?

@FenrirWulf FenrirWulf requested a review from hjpalpha July 16, 2024 20:18
@FenrirWulf FenrirWulf changed the title feat(match2): Adding MVPs to Clash Royale feat(match2): Refactring Clash Royale Match2 Jul 16, 2024
@hjpalpha hjpalpha marked this pull request as draft August 9, 2024 11:23
@hjpalpha
Copy link
Collaborator

hjpalpha commented Sep 3, 2024

needs retesting!

@Hesketh2
Copy link
Collaborator

Hesketh2 commented Sep 4, 2024

Pasting over onto |dev=1 - https://liquipedia.net/clashroyale/index.php?title=User:Hesketh2/test&action=edit&redlink=1

image

Seems working with the submatch variation but not as for Solo/Duos with cards

@Hesketh2
Copy link
Collaborator

Hesketh2 commented Sep 4, 2024

image
Display now works, but it's now not recognizing the score count (0-0 despite winner=X is filled)

same as previous one, Team Variation unaffected

@hjpalpha
Copy link
Collaborator

hjpalpha commented Sep 4, 2024

image Display now works, but it's now not recognizing the score count (0-0 despite winner=X is filled)

same as previous one, Team Variation unaffected

fixed

@Rathoz
Copy link
Collaborator

Rathoz commented Sep 9, 2024

#4650 adjustment needed

@hjpalpha hjpalpha marked this pull request as ready for review September 18, 2024 09:09
@hjpalpha hjpalpha requested a review from Rathoz September 18, 2024 09:09
@hjpalpha hjpalpha requested a review from Rathoz September 18, 2024 11:13
@Rathoz Rathoz merged commit 7e1bb88 into main Sep 19, 2024
5 checks passed
@Rathoz Rathoz deleted the CR-M2-MVPs branch September 19, 2024 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants