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

Add NaN logic to ASI flow #2494

Merged
merged 2 commits into from
Oct 23, 2023
Merged

Add NaN logic to ASI flow #2494

merged 2 commits into from
Oct 23, 2023

Conversation

Hoppyhob
Copy link
Contributor

Closes #2441

  • Added NaN Check to _onClickButton to reset assignments value to 0 if it is NaN or undefined.
  • Added NaN check to getData() to return the assignment to 0 if NaN.
  • Added check to set ability as fixed if ability's initial value matched the ability's max value.

Copy link
Contributor

@Fyorl Fyorl left a comment

Choose a reason for hiding this comment

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

Thanks for doing this work. In addition to the comment I left, I feel it would be preferable to prevent the value from ever becoming NaN at all rather than spreading NaN checks around our code. I believe the NaN value is originating in _onChangeInput where we call input.valueAsNumber. We should instead do something like this:

const value = isNaN(input.valueAsNumber) ? 0 : input.valueAsNumber;
const clampedValue = Math.clamped(value, Number(input.min), Number(input.max));

Closes foundryvtt#2441
- Added NaN Check to _onChangeInput to set assignments[key] to 0 if input.valueAsNumber is NaN otherwise it continues original logic.
- Added check to set ability as isfixed if ability's initial value is greater than or equal to the ability's max value.
@Hoppyhob
Copy link
Contributor Author

Hoppyhob commented Oct 20, 2023

Added the requested changes and removed the NaN checks that are now redundant.
For the _onChangeInput I added a NaN check that directly sets the assignments[key] to zero. (Instead of setting value to zero then clamping it to the minimum then finding the difference) Otherwise if not NaN it will go through the original logic.

@arbron arbron added this to the D&D5E 2.4.0 milestone Oct 21, 2023
Copy link
Contributor

@Fyorl Fyorl left a comment

Choose a reason for hiding this comment

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

Nice, thanks, looks good to me

@Fyorl Fyorl merged commit b7c924a into foundryvtt:2.4.x Oct 23, 2023
@Hoppyhob Hoppyhob deleted the 2.4.x_#2441 branch February 3, 2024 22:03
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.

3 participants