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

Donut utilization & threshold charts #2064

Merged
merged 2 commits into from May 29, 2019

Conversation

@dlabrecq
Copy link
Member

dlabrecq commented May 22, 2019

Added new donut utilization & threshold charts.

Fixes patternfly/patternfly-next#901

donut-util

Screen Shot 2019-05-23 at 12 55 00 PM

But wait, there's more...

donut-treshold

Screen Shot 2019-05-23 at 12 54 42 PM

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented May 22, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 22, 2019

Codecov Report

Merging #2064 into master will decrease coverage by 0.21%.
The diff coverage is 70.37%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2064      +/-   ##
=========================================
- Coverage   81.32%   81.1%   -0.22%     
=========================================
  Files         636     643       +7     
  Lines        7560    7716     +156     
  Branches      451     451              
=========================================
+ Hits         6148    6258     +110     
- Misses       1212    1258      +46     
  Partials      200     200
Flag Coverage Δ
#patternfly3 84.88% <ø> (ø) ⬆️
#patternfly4 76.93% <70.37%> (-0.28%) ⬇️
#patternflymisc 95.68% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...react-charts/src/components/ChartAxis/ChartAxis.js 100% <ø> (ø) ⬆️
...charts/src/components/ChartTooltip/ChartTooltip.js 100% <ø> (ø) ⬆️
...react-charts/src/components/ChartArea/ChartArea.js 100% <ø> (ø) ⬆️
...nts/ChartVoronoiContainer/ChartVoronoiContainer.js 80% <ø> (ø) ⬆️
...ts/src/components/ChartTheme/themes/theme-donut.js 100% <ø> (ø) ⬆️
...t-charts/src/components/ChartLegend/ChartLegend.js 100% <ø> (ø) ⬆️
...4/react-charts/src/components/ChartBar/ChartBar.js 100% <ø> (ø) ⬆️
...rnfly-4/react-charts/src/components/Chart/Chart.js 100% <ø> (ø) ⬆️
...4/react-charts/src/components/ChartPie/ChartPie.js 100% <ø> (ø) ⬆️
...act-charts/src/components/ChartStack/ChartStack.js 100% <ø> (ø) ⬆️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 288da47...841a4ad. Read the comment docs.

@dlabrecq dlabrecq force-pushed the dlabrecq:901-donut-threshold-charts branch 3 times, most recently from 39a33cb to 31962a0 May 23, 2019
@dlabrecq

This comment has been minimized.

Copy link
Member Author

dlabrecq commented May 23, 2019

Removed legendHeight and legendWidth props in favor of using a Victory's getDimensions function.

@dlabrecq dlabrecq force-pushed the dlabrecq:901-donut-threshold-charts branch from 31962a0 to d59efa9 May 23, 2019
@gdoyle1

This comment has been minimized.

Copy link

gdoyle1 commented May 23, 2019

@dlabrecq Can we change the naming of donut percentage to be "donut utilization"?

@dlabrecq

This comment has been minimized.

Copy link
Member Author

dlabrecq commented May 23, 2019

Sure, I've renamed as "donut utilization"

@dlabrecq dlabrecq force-pushed the dlabrecq:901-donut-threshold-charts branch from d59efa9 to 6a17ad7 May 23, 2019
@dlabrecq dlabrecq changed the title Donut percentage & threshold charts Donut utilization & threshold charts May 23, 2019
Copy link
Contributor

TheRealJon left a comment

Overall, this looks good. I just had a couple of concerns as stated in my comments:

  1. I don't think we should have 3 separate donut chart components. A standard donut and a utilization donut should be enough. The static threshold donut probably doesn't need to be broken out into a separate component since it will only ever be used in combination with the utilization donut. I know we want to lean towards composition, but it doesn't make sense to break out and export a component that only gets used in one place. I think the '80% use case' rule applies here to justify this as configuration rather than composition.

  2. The data prop expects an array of objects with x and y props, but does not require it to be in that format. We are using the data to make calculations but don't account for the fact that it could be arbitrary (i.e. x and y props might not exist on each piece of data). I think we will probably need to look at how Victory handles the data prop in other charts and try to mimic their logic. I'm guessing they probably have helper functions to extract values from the data prop.

  3. In the ChartUtilizationDonut, if the data prop contains more than one item, the graph will exhibit really strange behavior. We are doing some calculations to figure out the 'unused' portion of the graph, but don't handle the edge case where multiple data points are provided. There might need to be a discussion as to what the behavior should be in that case, but IMO it should be handled gracefully and still display something that makes sense. My suggestion is that take another prop capacity which will be used to calculate a percentage from arbitrary values. It can default to 100 so that if the user passes percentage values, it will just work as expected. If multiple values are passed, we can use the sum of all data values as the total percentage.

@dlabaj dlabaj self-requested a review May 24, 2019
@dlabrecq dlabrecq force-pushed the dlabrecq:901-donut-threshold-charts branch 3 times, most recently from 5d28935 to 798efd6 May 24, 2019
@dlabrecq dlabrecq force-pushed the dlabrecq:901-donut-threshold-charts branch from 798efd6 to 3a87926 May 25, 2019
@dlabrecq

This comment has been minimized.

Copy link
Member Author

dlabrecq commented May 25, 2019

Modified per code review comments.

After discussing with @TheRealJon and @gdoyle1, I've also moved the static threshold examples under donut utilization.

@dlabrecq dlabrecq force-pushed the dlabrecq:901-donut-threshold-charts branch 5 times, most recently from a95d21c to 57c8411 May 25, 2019
@dlabrecq dlabrecq force-pushed the dlabrecq:901-donut-threshold-charts branch 5 times, most recently from b33b455 to 0addfc3 May 26, 2019
@dlabrecq dlabrecq force-pushed the dlabrecq:901-donut-threshold-charts branch from 0addfc3 to ee972b5 May 28, 2019
@dlabaj

This comment has been minimized.

Copy link
Contributor

dlabaj commented May 28, 2019

Build is failing

@dlabrecq

This comment has been minimized.

Copy link
Member Author

dlabrecq commented May 28, 2019

Hmm, didn't change Chart.d.ts.

Can' reproduce it locally, but the build may be failing because we (literally a couple min, ago) merged changes to build typescript?

Screen Shot 2019-05-28 at 12 42 06 PM

@dlabrecq dlabrecq force-pushed the dlabrecq:901-donut-threshold-charts branch from ee972b5 to b650cd6 May 28, 2019
@gdoyle1

This comment has been minimized.

Copy link

gdoyle1 commented May 28, 2019

@dlabrecq Also Donut Utilization should be "Donut utilization" (sorry, nit-picky) to follow sentence case

@dlabrecq dlabrecq force-pushed the dlabrecq:901-donut-threshold-charts branch from 9209c32 to 4ca0758 May 29, 2019
@dlabrecq dlabrecq force-pushed the dlabrecq:901-donut-threshold-charts branch from 4ca0758 to 841a4ad May 29, 2019
Copy link
Contributor

TheRealJon left a comment

lgtm

@dlabaj
dlabaj approved these changes May 29, 2019
@dlabaj dlabaj merged commit 553f115 into patternfly:master May 29, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dlabrecq dlabrecq deleted the dlabrecq:901-donut-threshold-charts branch Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.