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

[Bug] Fix Flash Fire not showing message upon activation #4063

Open
wants to merge 8 commits into
base: beta
Choose a base branch
from

Conversation

JSLveil
Copy link

@JSLveil JSLveil commented Sep 6, 2024

What are the changes the user will see?

Flash Fire will now send a Fire-boosting message once upon activation and an immunity message every time after.

Why am I making these changes?

Closes #4058

What are the changes from a developer perspective?

  • data/ability: Added a condition in TypeImmunityAddBattlerTagAbAttr (which only Flash Fire uses but whatever) to check if the effect has already been applied. If no, display the new Flash Fire message and if yes, display the standard immunity message.
  • locales/en/ability-trigger: Added new Flash Fire message that generalizes to any move type if required.

Screenshots/Videos

Before
PreFix.mp4
After
AfterFix.mp4

How to test the changes?

Activate Flash Fire. I used overrides.

Checklist

  • I'm using beta as my base branch
  • I wasn't before though, whoops
  • There is no overlap with another PR?
  • The PR is self-contained and cannot be split into smaller PRs?
  • Have I provided a clear explanation of the changes?
  • If I have text, did I make it translatable and add a key in the English locale file(s)?
  • Not sure about this one actually, but I think I did everything right unless I'm forgetting something.
  • Have I tested the changes (manually)?
    • Are all unit tests still passing? (npm run test)
  • Are the changes visual?
    • Have I provided screenshots/videos of the changes?

@JSLveil JSLveil requested review from a team as code owners September 6, 2024 05:17
src/data/ability.ts Outdated Show resolved Hide resolved
@DayKev
Copy link
Collaborator

DayKev commented Sep 6, 2024

Uh, you forgot to add a description to the PR.

@DayKev DayKev added Ability Affects an ability P3 Bug Non gameplay affecting bug. typos, graphical issues, or other minor incorrect interactions. labels Sep 6, 2024
src/data/ability.ts Outdated Show resolved Hide resolved
@DayKev DayKev changed the base branch from main to beta September 6, 2024 05:30
Co-authored-by: NightKev <34855794+DayKev@users.noreply.github.com>
@JSLveil JSLveil changed the title [Bug] Fixed Flash Fire not showing message upon activation [Bug] Fix Flash Fire not showing message upon activation Sep 6, 2024
@DayKev DayKev linked an issue Sep 6, 2024 that may be closed by this pull request
src/data/ability.ts Outdated Show resolved Hide resolved
@SangaraSorama
Copy link
Contributor

The locales PR adding the line to flash fire is merged

@DayKev
Copy link
Collaborator

DayKev commented Oct 16, 2024

I think it's good, but apparently testing locales changes is difficult. Need to figure out how to force the locales update to appear locally...

@DayKev DayKev requested review from a team, CodeTappert, Opaque02, ben-lear and torranx and removed request for a team October 16, 2024 10:07
DayKev
DayKev previously approved these changes Nov 3, 2024
Copy link
Collaborator

@innerthunder innerthunder left a comment

Choose a reason for hiding this comment

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

Flash Fire's message should be contained within its respective tag (i.e. in the onAdd function). This implementation isn't really scalable since it only displays the right message when the attribute is used for Flash Fire

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ability Affects an ability P3 Bug Non gameplay affecting bug. typos, graphical issues, or other minor incorrect interactions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Flash Fire message does not display
5 participants