-
Notifications
You must be signed in to change notification settings - Fork 5
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
set min/max correctly when values are zero [MRXN23-559] #1631
set min/max correctly when values are zero [MRXN23-559] #1631
Conversation
Initial implementation would incorrectly trigger the branch of the ternary operator that ends up setting min or max to `null` for any falseish value (including numeric `0`). We should probably treat undefined `feature?.amountMin` or `feature?.amountMin` as error conditions, but that's something beyond the scope of the current change.
To avoid SLOC inflation in class, conflation of concerns, and to make it easier to unit-test logic.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
e164844
to
f3e89fb
Compare
@KevSanchez the issue here was a classical PEBKAC on my end:
This is obviously going to result in a loud sad-trombone fail sound when Since my being a bad person didn't stop at this bug but also at sticking more and more code inside of the already-inflated |
To be fair, I would have followed the exact same assumption; another stark reminder of the dangerous subtleties of javascript :\ |
Substitute this line for a meaningful title for your changes
Overview
Fix my own 🤦🏼 bug that broke stuff because of an improper reliance on falseish values.
Designs
N/A
Testing instructions
There's a new unit test file, which I believe captures most of the logic and edge cases, including the sad-trombone bug in my previous implementation.
Feature relevant tickets
https://vizzuality.atlassian.net/browse/MRXN23-559
Checklist before submitting
develop
.deploying to staging/production, please add brief testing instructions
to the deploy checklist (
docs/deployment-checklist.md
)