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(Donut Chart): Add fixed point notation into percentage donut charts #2375

Merged
merged 1 commit into from Jul 24, 2019

Conversation

@amirfefer
Copy link
Contributor

amirfefer commented Jun 27, 2019

donut chart rounds up the title which might lead to a falsy 100%

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 27, 2019

PatternFly-React preview: https://patternfly-react-pr-2375.surge.sh

@dlabaj
dlabaj approved these changes Jun 27, 2019
@dlabaj dlabaj self-assigned this Jun 27, 2019
@dlabaj dlabaj added the PF4 label Jun 27, 2019
@@ -15,13 +15,14 @@ const setDonutTitle = obj => {

const { props } = obj;
const { data, title = {} } = props;
const { type, percision = 0 } = title;

This comment has been minimized.

Copy link
@TheRealJon

TheRealJon Jun 27, 2019

Contributor
Suggested change
const { type, percision = 0 } = title;
const { type, precision = 0 } = title;
@dlabaj dlabaj requested a review from dlabrecq Jun 27, 2019
Copy link
Contributor

tlabaj left a comment

is there related issue for this PR if so can you please add it to the description

case 'percent':
primary = `${Math.round((100 * columns[iMax][1]) / sum).toString()}%`;
primary = `${((100 * columns[iMax][1]) / sum).toFixed(percision).toString()}%`;

This comment has been minimized.

Copy link
@TheRealJon

TheRealJon Jun 27, 2019

Contributor

Check out Intl.NumberFormat

Suggested change
primary = `${((100 * columns[iMax][1]) / sum).toFixed(percision).toString()}%`;
primary = `${Intl.NumberFormat(null, {style: 'percent', maximumFractionDigits: precision}).format(columns[iMax][1]) / sum)}%`;

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Jun 28, 2019

Member

Wouldn't this change the output for existing applications? Perhaps we can leave the existing code as is, unless the precision property is added?

}}
data={donutPercentageData}
tooltip={donutConfigTooltip}
title={{ type: 'percent', percision: 1 }}

This comment has been minimized.

Copy link
@TheRealJon

TheRealJon Jun 27, 2019

Contributor
Suggested change
title={{ type: 'percent', percision: 1 }}
title={{ type: 'percent', precision: 1 }}
Copy link
Contributor

TheRealJon left a comment

@dlabaj I think using fixed-point notation would lead to decimal places being displayed, regardless of whether the value is fractional. (e.g. If percentage is 100, precision =1 gives you 100.0%, precision = 2 gives you 100.00%, and so on).

@tlabaj tlabaj added PF3 bug 🐛 and removed PF4 labels Jun 27, 2019
@TheRealJon

This comment has been minimized.

Copy link
Contributor

TheRealJon commented Jun 27, 2019

@dlabaj I think using fixed-point notation would lead to decimal places being displayed, regardless of whether the value is fractional. (e.g. If percentage is 100, precision =1 gives you 100.0%, precision = 2 gives you 100.00%, and so on).

ping @amirfefer

case 'percent':
primary = `${Math.round((100 * columns[iMax][1]) / sum).toString()}%`;
primary = `${((100 * columns[iMax][1]) / sum).toFixed(percision).toString()}%`;

This comment has been minimized.

Copy link
@ares

ares Jun 28, 2019

actually this may still be a problem - (100 * 99.99999999 / 100).toFixed(2) # => 100.00
I'm not sure if the other suggestion solves it, maybe it does

case 'percent':
primary = `${Math.round((100 * columns[iMax][1]) / sum).toString()}%`;
primary = `${((100 * columns[iMax][1]) / sum).toFixed(percision).toString()}%`;

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Jun 28, 2019

Member

Wouldn't this change the output for existing applications? Perhaps we can leave the existing code as is, unless the precision property is added?

Copy link
Contributor

dlabaj left a comment

Agree with @dlabrecq can we require the user to use the precision property if they want this... also is there an issue related to this fix. If not can we create one and reference it.

@ares

This comment has been minimized.

Copy link

ares commented Jul 3, 2019

I'm not sure if this is ever wanted behavior (in fact it may be seen as a bug), @amirfefer, what do you think? I'm fine with configuring through a property.

@amirfefer amirfefer force-pushed the amirfefer:add-fixed-point-notation branch from c2972a2 to 677fe97 Jul 9, 2019
@amirfefer

This comment has been minimized.

Copy link
Contributor Author

amirfefer commented Jul 9, 2019

@ares, toFixed apparently rounds up, so I use the known slice function.
@dlabaj - there is an issue for this already - #2356
@TheRealJon - Intl.NumberFormat doesn't work here - getting Cannot convert undefined or null to object, slicing is simpler in this case IMO.
plus, if precision prop doesn't exist, the previous functionality remains

donut chart rounds up the title which might lead to a falsy 100%
@amirfefer amirfefer force-pushed the amirfefer:add-fixed-point-notation branch from 677fe97 to 563d481 Jul 9, 2019
@ares

This comment has been minimized.

Copy link

ares commented Jul 23, 2019

ping, is there something we can do to get this merged?

@dlabrecq dlabrecq requested review from dlabrecq and TheRealJon Jul 23, 2019
@dlabrecq

This comment has been minimized.

Copy link
Member

dlabrecq commented Jul 23, 2019

I see that the precision prop was added, so I'm good with that. I'd like to give @TheRealJon another chance to comment before merging.

@dlabaj
dlabaj approved these changes Jul 23, 2019
@ares

This comment has been minimized.

Copy link

ares commented Jul 23, 2019

Awesome I see 2 acks now :-)

@tlabaj
tlabaj approved these changes Jul 24, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit 937d2d7 into patternfly:master Jul 24, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jul 24, 2019

Your changes have been released in:

  • patternfly-react-extensions@2.19.7
  • patternfly-react@2.36.7
  • @patternfly/react-console@1.11.7

Thanks for your contribution! 🎉

@ares

This comment has been minimized.

Copy link

ares commented Jul 25, 2019

Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.