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

fix(ui): Prevent negative zero and allow zero with decimal places #16439

Merged
merged 5 commits into from
Jan 8, 2020

Conversation

TCL735
Copy link
Contributor

@TCL735 TCL735 commented Jan 7, 2020

Closes #16306

  • Prevent negative zero
  • Allow zero to have decimal places

@TCL735 TCL735 requested a review from a team January 7, 2020 23:39
@ghost ghost requested review from asalem1 and hoorayimhelping and removed request for a team January 7, 2020 23:39
@TCL735 TCL735 requested a review from zoesteinkamp January 7, 2020 23:39
import {DecimalPlaces} from 'src/types/dashboards'

// Constants
import {MAX_DECIMAL_PLACES} from 'src/dashboards/constants'
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice job organizing these

export const preventNegativeZero = (
value: number | string
): number | string => {
if (Number(value) === -0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Literally didn't know this was possible

Copy link
Contributor

@hoorayimhelping hoorayimhelping left a comment

Choose a reason for hiding this comment

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

take control back from the machines!

no but seriously, go ahead and use overrides here to get more value out of our tests. in general, use an override like this for lint or prettier or whatever, whenever they get mouthy and you think what you're writing is valuable.

maximumFractionDigits: MAX_DECIMAL_PLACES,
})
localeFormattedValue =
Number(roundedValue) === 0
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think of turning this code into a formatting function and including the negative zero check as part of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This piece of code is actually inside a formatting function already. The ternary is to allow positive zero to have decimal places when user-specified.

Copy link
Contributor

Choose a reason for hiding this comment

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

well then

ui/src/shared/utils/preventNegativeZero.test.ts Outdated Show resolved Hide resolved
@TCL735 TCL735 requested a review from drdelambre January 7, 2020 23:48
@TCL735 TCL735 requested a review from hoorayimhelping January 7, 2020 23:56
Copy link
Contributor

@hoorayimhelping hoorayimhelping left a comment

Choose a reason for hiding this comment

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

Nice work!

import {preventNegativeZero} from 'src/shared/utils/preventNegativeZero'

describe('preventNegativeZero', () => {
it('should not alter non-zero numbers', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice test! was going to ask why you can't just use Math.abs but this one answers it nicely.

@TCL735 TCL735 merged commit 578f97d into master Jan 8, 2020
@TCL735 TCL735 deleted the fix_16306_negative_zero branch January 8, 2020 00:24
alexpaxton pushed a commit that referenced this pull request Jan 9, 2020
…6439)

* fix(ui): Prevent negative zero and allow zero with decimal places

* chore: update CHANGELOG

* fix(ui): Turn off lint rule no-compare-neg-zero

* test: add more tests by overriding prettier

* fix(ui): turn off lint rule for a specific usage, not universally
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

-0 displayed in single stat for -0.001 with truncated decimals
4 participants