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(charts): align label vertically and add donutHeight/Width defaults #2193

Merged
merged 2 commits into from Jun 10, 2019

Conversation

@dlabrecq
Copy link
Member

dlabrecq commented Jun 8, 2019

This PR is for donut utilization changes requested by @TheRealJon for use in OpenShift.

Fixes #2191
Fixes #2192
Fixes #2194

I was able to fix #2191 by setting capHeight (using a rem value) in order to help center the label vertically. I only use this when both title and subtitle are available -- not necessary for a single label. I've added a property so it may be overridden.

Screen Shot 2019-06-07 at 8 39 09 PM

I was able to address #2192 by changing the default donutHeight and donutWidth properties to use the min. of the given SVG height or width. If height & width are not provided, the default theme property would be used.

With these changes, the user can just set the height and width properties. Similar to other charts, we'll set the donut size to match that.

This also works with the threshold chart. I've modified the dynamic chart to be 28px smaller than the static (threshold) chart by default. This is similar to innerRadius, where we set a default regardless of the chart size.

This probably covers most use cases where the width is larger to accommodate either a left or right aligned legend. Of course, it also works without any legends at all. Thus, allowing the user to more easily create different size charts.

small-donuts

For edge cases, the user can still override the donutHeight and donutWidth properties. There an example of this (below), where the overall SVG height and width are larger than donutHeight and donutWidth. In this particular example, the horizontal legend is wider than the chart's donutWidth, while the SVG is sized appropriately to accommodate both the chart and legend.

Screen Shot 2019-06-08 at 9 17 37 AM

@dlabrecq dlabrecq requested a review from TheRealJon Jun 8, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 8, 2019

@dlabrecq dlabrecq force-pushed the dlabrecq:2191-2192-charts branch from 9933996 to 503351d Jun 8, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 8, 2019

Codecov Report

Merging #2193 into master will decrease coverage by 0.2%.
The diff coverage is 4.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2193      +/-   ##
==========================================
- Coverage   80.65%   80.45%   -0.21%     
==========================================
  Files         660      660              
  Lines        8322     8343      +21     
  Branches      628      641      +13     
==========================================
  Hits         6712     6712              
- Misses       1310     1331      +21     
  Partials      300      300
Flag Coverage Δ
#patternfly3 85.22% <ø> (ø) ⬆️
#patternfly4 75.78% <4.08%> (-0.38%) ⬇️
#patternflymisc 95.68% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...ents/ChartDonutUtilization/ChartDonutThreshold.tsx 59.75% <4%> (-3.07%) ⬇️
...ts/ChartDonutUtilization/ChartDonutUtilization.tsx 59.09% <4.16%> (-10.81%) ⬇️

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 979f13f...8390c82. Read the comment docs.

@dlabrecq dlabrecq force-pushed the dlabrecq:2191-2192-charts branch from 503351d to fd24990 Jun 8, 2019
@dlabrecq dlabrecq force-pushed the dlabrecq:2191-2192-charts branch from f413607 to 8390c82 Jun 9, 2019
@rachael-phillips rachael-phillips added the p1 label Jun 10, 2019
Copy link
Contributor

TheRealJon left a comment

LGTM

@tlabaj
tlabaj approved these changes Jun 10, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@dlabaj
dlabaj approved these changes Jun 10, 2019
@dlabaj dlabaj merged commit 59aadfc into patternfly:master Jun 10, 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:2191-2192-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
7 participants
You can’t perform that action at this time.