feat(bullet-chart): Add the bullet-chart component #1061

jeff-phillips-18 · 2018-05-23T14:37:40Z

Adds the html/css implementation of
http://www.patternfly.org/pattern-library/data-visualization/bullet-chart/#design

Please not this was implemented to include patternfly/patternfly-design#662

Link to rawgit and/or image

https://rawgit.com/jeff-phillips-18/patternfly/bullet-chart-dist/dist/tests/bullet-charts.html

PR checklist (if relevant)

Cross browser: works in IE10
Cross browser: works in IE11
Cross browser: works in Edge
Cross browser: works in Chrome
Cross browser: works in Firefox
Cross browser: works in Safari
Cross browser: works in Opera
Responsive: works in extra small, small, medium and large view ports.
Preview the PR: An image or a URL for designer to preview this PR is provided.

jeff-phillips-18 · 2018-05-23T14:38:38Z

@serenamarie125

mcoker · 2018-05-23T19:41:45Z

tests/pages/bullet-charts.html

+          <div class="bullet-chart-pf-title-spacer"></div>
+          <div class="bullet-chart-pf-title-labels-container">
+            <div class="bullet-chart-pf-title">
+              Text Label</div>


Just a formatting thing, but the closing </div> could be moved to the next line to be consistent with the .bullet-chart-pf-title/.bullet-chart-pf-details block above. The above block and this one could also be changed so each <div>, the text and the closing </div> are on a single line to match the title/detail blocks below (or the title/detail blocks below could be expanded to 3 lines like this one and the block above)

mcoker · 2018-05-23T19:43:59Z

I'm probably just out of the loop on this pattern, but why aren't the orange and red data lines included in the legend?

jeff-phillips-18 · 2018-05-24T14:24:41Z

I don't believe the thresholds are supposed to be represented in the legend. @serenamarie125 ?

serenamarie125

@jeff-phillips-18 @michael-coker although the design documentation does show it in the legend by default, from a space perspective when utilizing a bullet chart, there won't be room for all of that in most situations. IMO, the default should just be to show the values & ranges in the legend. If people want to add the thresholds, they should be able to override.

serenamarie125 · 2018-05-24T14:52:53Z

@kybaker can you provide input on the legend spacing? I know there are conventions because we have legends in our charts, but can't seem to find any guidance on that

jennyhaines · 2018-05-24T15:57:58Z

@jeff-phillips-18 - I think everything about the design looks good, except the legend spacing (that @serenamarie125 mentioned.) Here are is the spacing that should be used:

The legend should be horizontally centered underneath the horizontal chart, and vertically centered to the right of vertical charts
There should not be strokes around each colored legend tile
There should be 7px of spacing between each corresponding tile and descriptor element within a pair
There should be 10px of spacing between each tile/descriptor pair
There should be 13px of spacing between the edge of the chart and the legend

jeff-phillips-18 · 2018-05-25T11:16:17Z

Updated to comments above. @jennyhaines please take another look.

jennyhaines · 2018-05-25T15:01:36Z

@jeff-phillips-18 - this looks perfect to me!

jeff-phillips-18 · 2018-05-25T15:07:39Z

@michael-coker @matthewcarleton Can you take another look now that the visuals are correct?

serenamarie125

LGTM!

mcoker · 2018-05-25T15:58:33Z

@jeff-phillips-18 I'm assuming it's OK that the legend items wrap? And I see this...

.bullet-chart-pf-legend-item {
    display: inline-block;
    margin-right: 10px;
    overflow: hidden;
    text-overflow: ellipsis;
    white-space: nowrap;
    word-wrap: normal;
}

But that item doesn't appear to overflow or show ellipses, it will just grow to match the content.

jeff-phillips-18 · 2018-05-25T16:04:21Z

Correct. We could add a max-width if we want to prevent it from getting too long. Thoughts?

jeff-phillips-18 · 2018-05-25T16:11:07Z

Checked with @serenamarie125 and @jennyhaines, will set a max-width of 150px.

jeff-phillips-18 · 2018-05-25T16:21:46Z

Updated, thanks!

mcoker · 2018-05-25T16:22:55Z

The horizontal .bullet-chart-pf-legend is a block element with text-align: center; with <span> spacer elements set to display: none;, and the vertical layout is a flex column with spacers on either side that vertically center the .bullet-chart-pf-legend-item items.

What about making both the horizontal and vertical versions display: flex;, remove the spacers and use justify-content: center; to center the .bullet-chart-pf-legend-item items, and set the horizontal version to flex-wrap: wrap;?

serenamarie125

LGTM @jeff-phillips-18

mcoker · 2018-05-25T18:01:40Z

src/less/charts.less

+  border-style: solid;
+  border-width: 10px;
+  height: 20px;
+  margin-left: -10px;


I would use transform: translateX(-50%); here just to avoid fixed units whenever possible. And change margin-left: 0; on the vertical CSS below to transform: translateX(0);

mcoker · 2018-05-25T18:02:21Z

src/less/charts.less

+}
+
+.bullet-chart-pf-value-dot {
+  border-radius: 10px;


Use border-radius: 50%; to avoid fixed units whenever possible.

mcoker · 2018-05-25T18:27:55Z

src/less/charts.less

+.bullet-chart-pf-value-dot {
+  border-radius: 10px;
+  border-style: solid;
+  border-width: 10px;


Is there a reason you're using border properties instead of background-color to define the dot color?

mcoker · 2018-05-25T18:40:34Z

src/less/charts.less

+.bullet-chart-pf {
+  display: flex;
+  flex-direction: column;
+  width: 100%;


This looks unnecessary, the element should grow to fill the available space by default.

mcoker · 2018-05-25T18:48:01Z

src/less/charts.less

+  height: 20px;
+  margin: 20px 0;
+  position: relative;
+  width: 100%;


This looks unnecessary, the element should grow to fill the available space by default.

mcoker · 2018-05-25T18:50:33Z

src/less/charts.less

+.bullet-chart-pf-legend {
+  margin-top: 7px;
+  text-align: center;
+  width: 100%;


This (and width: initial; on the vertical version below) look unnecessary.

mcoker · 2018-05-29T19:33:03Z

src/less/charts.less

+    padding-right: 0;
+    text-align: center;
+  }
+}


To center this text relative to the chart (and not the axis below itchange this .bullet-chart-pf-title-labels-container block to this. Basically just create 25px of space at the bottom of the container to match the height of the axis row.

.bullet-chart-pf-title-labels-container { margin: 0 10px 25px 0; text-align: right; .bullet-chart-pf-vertical & { text-align: center; margin: 10px 0; } }

mcoker · 2018-05-29T19:33:47Z

tests/pages/bullet-charts.html

+            <div class="bullet-chart-pf-details">Measure Details</div>
+          </div>
+          <!-- add this div to vertically center the titles -->
+          <div class="bullet-chart-pf-axis"></div>


You can remove this if you apply the CSS changes previously mentioned

mcoker · 2018-05-29T19:34:00Z

tests/pages/bullet-charts.html

+            <div class="bullet-chart-pf-details">Measure Details</div>
+          </div>
+          <!-- add this div to vertically center the titles -->
+          <div class="bullet-chart-pf-axis"></div>


you can also remove this

jeff-phillips-18 · 2018-05-30T16:15:40Z

Updated. Thanks for the simplifications @michael-coker !

Adds the html/css implementation of http://www.patternfly.org/pattern-library/data-visualization/bullet-chart/#design

jeff-phillips-18 requested a review from matthewcarleton May 23, 2018 14:37

jeff-phillips-18 requested a review from mcoker May 23, 2018 16:48

mcoker reviewed May 23, 2018

View reviewed changes

serenamarie125 reviewed May 24, 2018

View reviewed changes

mcoker previously approved these changes May 24, 2018

View reviewed changes

jeff-phillips-18 dismissed mcoker’s stale review via e13ff9c May 24, 2018 17:36

serenamarie125 previously approved these changes May 25, 2018

View reviewed changes

jeff-phillips-18 dismissed serenamarie125’s stale review via 52ca4f1 May 25, 2018 16:20

jeff-phillips-18 force-pushed the bullet-chart branch from e13ff9c to 52ca4f1 Compare May 25, 2018 16:20

jeff-phillips-18 force-pushed the bullet-chart branch from fd4beab to 588922a Compare May 29, 2018 11:44

serenamarie125 previously approved these changes May 29, 2018

View reviewed changes

mcoker reviewed May 30, 2018

View reviewed changes

jeff-phillips-18 dismissed serenamarie125’s stale review via c652a23 May 30, 2018 16:15

jeff-phillips-18 force-pushed the bullet-chart branch from c652a23 to 049e2d6 Compare May 30, 2018 16:19

mcoker previously approved these changes May 30, 2018

View reviewed changes

serenamarie125 previously approved these changes May 30, 2018

View reviewed changes

feat(bullet-chart): Add the builet-chart component

73be693

Adds the html/css implementation of http://www.patternfly.org/pattern-library/data-visualization/bullet-chart/#design

jeff-phillips-18 dismissed stale reviews from serenamarie125 and mcoker via 73be693 May 30, 2018 19:00

jeff-phillips-18 force-pushed the bullet-chart branch from e0998e5 to 73be693 Compare May 30, 2018 19:00

mcoker approved these changes May 30, 2018

View reviewed changes

mcoker merged commit 4c69eb5 into patternfly:master May 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bullet-chart): Add the bullet-chart component #1061

feat(bullet-chart): Add the bullet-chart component #1061

jeff-phillips-18 commented May 23, 2018

jeff-phillips-18 commented May 23, 2018

mcoker May 23, 2018

mcoker commented May 23, 2018

jeff-phillips-18 commented May 24, 2018

serenamarie125 left a comment

serenamarie125 commented May 24, 2018

jennyhaines commented May 24, 2018

jeff-phillips-18 commented May 25, 2018

jennyhaines commented May 25, 2018

jeff-phillips-18 commented May 25, 2018

serenamarie125 left a comment

mcoker commented May 25, 2018

jeff-phillips-18 commented May 25, 2018

jeff-phillips-18 commented May 25, 2018

jeff-phillips-18 commented May 25, 2018

mcoker commented May 25, 2018

serenamarie125 left a comment

mcoker May 25, 2018

mcoker May 25, 2018

mcoker May 25, 2018

mcoker May 25, 2018

mcoker May 25, 2018

mcoker May 25, 2018

mcoker May 29, 2018

mcoker May 29, 2018

mcoker May 29, 2018

jeff-phillips-18 commented May 30, 2018

feat(bullet-chart): Add the bullet-chart component #1061

feat(bullet-chart): Add the bullet-chart component #1061

Conversation

jeff-phillips-18 commented May 23, 2018

Link to rawgit and/or image

PR checklist (if relevant)

jeff-phillips-18 commented May 23, 2018

Choose a reason for hiding this comment

mcoker commented May 23, 2018

jeff-phillips-18 commented May 24, 2018

serenamarie125 left a comment

Choose a reason for hiding this comment

serenamarie125 commented May 24, 2018

jennyhaines commented May 24, 2018

jeff-phillips-18 commented May 25, 2018

jennyhaines commented May 25, 2018

jeff-phillips-18 commented May 25, 2018

serenamarie125 left a comment

Choose a reason for hiding this comment

mcoker commented May 25, 2018

jeff-phillips-18 commented May 25, 2018

jeff-phillips-18 commented May 25, 2018

jeff-phillips-18 commented May 25, 2018

mcoker commented May 25, 2018

serenamarie125 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-phillips-18 commented May 30, 2018