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(ChartDonutThreshold): Don't show static threshold donut tooltips by default. #2270

Merged
merged 1 commit into from Jun 17, 2019

Conversation

@TheRealJon
Copy link
Contributor

TheRealJon commented Jun 13, 2019

Pie chart tooltips display the x value of each slice as a tooltip label by default. Because we are
doing calculations to mutate the data, this can lead to array indices being displayed as slice labels. Change defaults so that no slice labels are displayed for the static threshold donut. Also, fix a bug where ChartDonutThreshold accessors were being used to format child ChartDonutUtilization data.

Fixes #2258
Fixes #2259

@TheRealJon TheRealJon requested a review from dlabrecq Jun 13, 2019
@TheRealJon TheRealJon force-pushed the TheRealJon:issue-2258 branch from 632ab8d to 47de986 Jun 13, 2019
}
]
];
}, [0, []]);

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Jun 14, 2019

Member

Should we apply something like this to donut utilization as well?

This comment has been minimized.

Copy link
@TheRealJon

TheRealJon Jun 14, 2019

Author Contributor

I almost did but thought it was probably outside of the scope of this PR.

This comment has been minimized.

Copy link
@TheRealJon

TheRealJon Jun 14, 2019

Author Contributor

We could probably pull out the utilization/threshold data formatting function into a single util function. The logic is very similar, we just need to handle the difference between a single utilization data point and multiple threshold data points.

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Jun 15, 2019

Member

Let's keep them separate. The code is complex and not easy to understand what's happening.

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 14, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 14, 2019

Codecov Report

Merging #2270 into master will decrease coverage by 0.02%.
The diff coverage is 21.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2270      +/-   ##
==========================================
- Coverage   79.89%   79.86%   -0.03%     
==========================================
  Files         669      669              
  Lines        8529     8524       -5     
  Branches      734      734              
==========================================
- Hits         6814     6808       -6     
- Misses       1362     1363       +1     
  Partials      353      353
Flag Coverage Δ
#patternfly3 85.23% <ø> (ø) ⬆️
#patternfly4 74.87% <21.05%> (-0.06%) ⬇️
#patternflymisc 95.79% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...ents/ChartDonutUtilization/ChartDonutThreshold.tsx 55.84% <21.05%> (-3.92%) ⬇️

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 eb0d436...47de986. Read the comment docs.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 14, 2019

Codecov Report

Merging #2270 into master will increase coverage by 0.04%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2270      +/-   ##
==========================================
+ Coverage   80.62%   80.66%   +0.04%     
==========================================
  Files         666      666              
  Lines        8427     8415      -12     
  Branches      711      704       -7     
==========================================
- Hits         6794     6788       -6     
+ Misses       1280     1274       -6     
  Partials      353      353
Flag Coverage Δ
#patternfly3 85.23% <ø> (ø) ⬆️
#patternfly4 76.27% <33.33%> (+0.07%) ⬆️
#patternflymisc 95.79% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...ents/ChartDonutUtilization/ChartDonutThreshold.tsx 61.42% <33.33%> (+1.67%) ⬆️

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 b30caa4...2b9a739. Read the comment docs.

@TheRealJon TheRealJon force-pushed the TheRealJon:issue-2258 branch from 47de986 to eb373b9 Jun 14, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 14, 2019

…by default.

Pie chart tooltips display the x value of each slice as a tooltip label by default. Because we are
doing our own calculations on the data and manually setting the x-value, this can lead to tooltips
displaying array index values. Also, fix a bug where ChartDonutThreshold accessors were being used
for child ChartDonutUtilization data prop.
@TheRealJon TheRealJon force-pushed the TheRealJon:issue-2258 branch from eb373b9 to 2b9a739 Jun 14, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj
tlabaj approved these changes Jun 17, 2019
@tlabaj tlabaj merged commit 7d9bd16 into patternfly:master Jun 17, 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
@TheRealJon TheRealJon deleted the TheRealJon:issue-2258 branch Jun 26, 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.