Fixes #23598 - move report metrics chart to c3 #5563

lizagilman · 2018-05-15T14:09:31Z

Using pf-react donut chart instead of flot

theforeman-bot · 2018-05-15T14:09:38Z

amirfefer

Thanks @lizagilman
I left some inline comments.

We have a component for a chart box with this design:

although I know that this page uses chart bar which still using flot.

amirfefer · 2018-05-15T14:13:15Z

app/views/config_reports/_metrics.html.erb

-      <div style="margin-top:50px;padding-bottom: 40px;">
-        <%= flot_pie_chart("metrics" ,_("Report Metrics"), metrics, :class => "statistics-pie small")%>
-      </div>
+      <div id="metrics_chart" class='metrics-chart' style="margin-top:50px;padding-bottom: 40px;"></div>


why not add the inline style to the metrics-chart class?

amirfefer · 2018-05-15T14:14:04Z

app/views/config_reports/_metrics.html.erb

@@ -18,7 +17,7 @@
  <div class="col-md-4">
    <table class="<%= table_css_classes %>" style="height: 398px;">
      <tbody>
-        <% metrics.sort.each do |title, value|%>
+      <% metrics.sort.each do |title, value|%>


indentation should be 2 space

amirfefer · 2018-05-15T14:14:57Z

app/views/config_reports/_metrics.html.erb

-        <tr>
-          <th><%= _("Total") %></th><th><%= metric (@totaltime || @config_report.runtime) %></th>
-        </tr>
+      <tr>


amirfefer · 2018-05-15T14:20:48Z

app/views/config_reports/_metrics.html.erb

-        <%= flot_pie_chart("metrics" ,_("Report Metrics"), metrics, :class => "statistics-pie small")%>
-      </div>
+      <div id="metrics_chart" class='metrics-chart' style="margin-top:50px;padding-bottom: 40px;"></div>
+      <%= mount_react_component('DonutChart', '#metrics_chart', metrics.to_a) %>


data should be in json format (to_json)

lizagilman · 2018-05-15T14:56:14Z

@amirfefer thanks for the quick review, updated.
should we move only one chart to a box? once @ripcurld is finished with the bar chart we can move both to chartbox

amirfefer

Thanks @lizagilman , just one minor issue with the css.

Generally speaking, We do test the component behavior,
but we don't test if a component actually is rendered in a page (end to end test).
do we want this kind of test?

amirfefer · 2018-05-16T12:51:38Z

app/assets/stylesheets/charts.scss

+.metrics-chart {
+  width: 240px;
+  margin: 12px auto;
+  margin-top: 50px;


duplicate margin-top with 12px and 50px values.
maybe use margin with 4 values?

lizagilman · 2018-05-16T15:21:26Z

@amirfefer thanks, css updated
I tried to test for the chart rendering both on integration test and controller test but it seems the container div is empty (the component is not mounted to #metrics_chart). Is this a normal behavior?

amirfefer · 2018-05-17T08:59:26Z

@lizagilman, in order to run js in integration test you should use js driver, tests with js end with _js_test, and inherit IntegrationTestWithJavascript.

lizagilman · 2018-05-17T10:27:01Z

@amirfefer ok, I added a test
Is this what you had in mind?

ohadlevy · 2018-05-17T13:23:37Z

@lizagilman mind adding an updated screenshot? thanks.

amirfefer · 2018-05-17T16:59:33Z

test/integration/config_report_js_test.rb

+
+class ConfigReportJSTest < IntegrationTestWithJavascript
+  test "should display report metrics chart" do
+    @report = ConfigReport.import(read_json_fixture('reports/applied.json'))


I think @ is redundant here

lizagilman · 2018-05-21T12:06:23Z

updated -

amirfefer

LGTM 👍

ohadlevy · 2018-05-22T09:44:43Z

this looks like a solid change, the only thing I'm a bit uncomfortable with is the red (and to some degree green) color inside the chart, in your screenshots it looks like something requires attention, when i looked at it on some other sample data it didnt look as bad, but it still feels wrong to me, @Rohoover any thoughts?

amirfefer · 2018-05-22T10:24:48Z

@ohadlevy: these colors are suggested by pf color-palette and mentioned here
But I tend to agree when it's about two distinct colors, the red color has some negative impact.
We can change the color pattern order, and move the red color from the second place.

Rohoover · 2018-05-22T11:53:22Z

@amirfefer Is correct. There is some flexibility as the donut doesn't specify which colors and only gives guidance to use the recommended PF Color palette. We can technically choose from any of those colors.

ohadlevy · 2018-05-22T12:06:18Z

so in this case would you recommend changing the colors or its just me?

…

On Tue, May 22, 2018 at 2:53 PM, Roxanne Hoover ***@***.***> wrote: @amirfefer <https://github.com/amirfefer> Is correct. There is some flexibility as the donut doesn't specify which colors and only gives guidance to use the recommended PF Color palette <http://www.patternfly.org/styles/color-palette/>. We can technically choose from any of those colors. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5563 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOx0zXmL4VInsl4WwmtDwwMWRDFPxnks5t0_wygaJpZM4T_n3V> .

Rohoover · 2018-05-22T12:29:34Z

@ohadlevy

It's you. ;)

In all seriousness, I am not married to any particular color choice at this point. I think PF does use the red, orange, green colors as both color and status in the donuts, which can be a bit confusing.

ares · 2018-05-23T06:16:47Z

I'd also prefer to avoid red if that's easy to do. But I can live with it :-)

lizagilman · 2018-05-23T09:30:52Z

How about now?

red is last

or we can replace it with a different color -

for now, pr updated with option 1 (red at last place)

@ohadlevy @ares do you prefer to avoid red completely?

ares · 2018-05-23T19:01:21Z

I prefer option 2, but not strong opinion. If red helps color-blind people, I'm fine with keeping it.

lizagilman · 2018-06-03T19:48:40Z

@tstrachota any preferences? or can we merge?

amirfefer

I prefer option 2 as well,
Please pick another color from this list.

lizagilman · 2018-06-05T13:15:05Z

ok, changed -

boaz0 · 2018-06-05T16:44:22Z

LGTM 👍

test failures not related.
[test foreman]
[test katello]

lizagilman · 2018-06-06T06:56:02Z

[test foreman]

tbrisker

Tested, works well. Thanks @lizagilman !

theforeman-bot added Needs testing Not yet reviewed labels May 15, 2018

amirfefer suggested changes May 15, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels May 15, 2018

lizagilman force-pushed the metrics-pie branch from f1635f4 to b805032 Compare May 15, 2018 14:46

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels May 15, 2018

lizagilman force-pushed the metrics-pie branch 2 times, most recently from 3a151c5 to 54ca054 Compare May 15, 2018 14:55

amirfefer suggested changes May 16, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels May 16, 2018

lizagilman force-pushed the metrics-pie branch from 54ca054 to fb9a08d Compare May 16, 2018 14:18

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels May 16, 2018

lizagilman force-pushed the metrics-pie branch from fb9a08d to e0290fd Compare May 17, 2018 10:24

amirfefer suggested changes May 17, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels May 17, 2018

lizagilman force-pushed the metrics-pie branch from e0290fd to 5c17b35 Compare May 18, 2018 11:08

theforeman-bot removed the Waiting on contributor label May 18, 2018

lizagilman force-pushed the metrics-pie branch from da41c7e to 8700818 Compare May 21, 2018 12:06

amirfefer previously approved these changes May 22, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label May 22, 2018

lizagilman dismissed amirfefer’s stale review via ef08df4 May 23, 2018 09:30

lizagilman force-pushed the metrics-pie branch from 8700818 to ef08df4 Compare May 23, 2018 09:30

lizagilman force-pushed the metrics-pie branch from ef08df4 to 03d7334 Compare May 23, 2018 12:03

amirfefer suggested changes Jun 5, 2018

View reviewed changes

theforeman-bot added the Waiting on contributor label Jun 5, 2018

Fixes #23598 - move report metrics chart to c3

da2a31c

lizagilman force-pushed the metrics-pie branch from 03d7334 to da2a31c Compare June 5, 2018 13:14

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jun 5, 2018

amirfefer approved these changes Jun 6, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Jun 6, 2018

tbrisker approved these changes Jun 10, 2018

View reviewed changes

tbrisker merged commit be6d3ed into theforeman:develop Jun 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #23598 - move report metrics chart to c3 #5563

Fixes #23598 - move report metrics chart to c3 #5563

lizagilman commented May 15, 2018

theforeman-bot commented May 15, 2018

amirfefer left a comment •

edited

Loading

amirfefer May 15, 2018

amirfefer May 15, 2018

amirfefer May 15, 2018

amirfefer May 15, 2018

lizagilman commented May 15, 2018 •

edited

Loading

amirfefer left a comment

amirfefer May 16, 2018

lizagilman commented May 16, 2018

amirfefer commented May 17, 2018

lizagilman commented May 17, 2018

ohadlevy commented May 17, 2018

amirfefer May 17, 2018

lizagilman May 21, 2018

lizagilman commented May 21, 2018 •

edited

Loading

amirfefer left a comment

ohadlevy commented May 22, 2018

amirfefer commented May 22, 2018 •

edited

Loading

Rohoover commented May 22, 2018

ohadlevy commented May 22, 2018 via email

Rohoover commented May 22, 2018

ares commented May 23, 2018

lizagilman commented May 23, 2018 •

edited

Loading

ares commented May 23, 2018

lizagilman commented Jun 3, 2018

amirfefer left a comment

lizagilman commented Jun 5, 2018

boaz0 commented Jun 5, 2018

lizagilman commented Jun 6, 2018

tbrisker left a comment

Fixes #23598 - move report metrics chart to c3 #5563

Fixes #23598 - move report metrics chart to c3 #5563

Conversation

lizagilman commented May 15, 2018

theforeman-bot commented May 15, 2018

amirfefer left a comment • edited Loading

Choose a reason for hiding this comment

amirfefer May 15, 2018

Choose a reason for hiding this comment

amirfefer May 15, 2018

Choose a reason for hiding this comment

amirfefer May 15, 2018

Choose a reason for hiding this comment

amirfefer May 15, 2018

Choose a reason for hiding this comment

lizagilman commented May 15, 2018 • edited Loading

amirfefer left a comment

Choose a reason for hiding this comment

amirfefer May 16, 2018

Choose a reason for hiding this comment

lizagilman commented May 16, 2018

amirfefer commented May 17, 2018

lizagilman commented May 17, 2018

ohadlevy commented May 17, 2018

amirfefer May 17, 2018

Choose a reason for hiding this comment

lizagilman May 21, 2018

Choose a reason for hiding this comment

lizagilman commented May 21, 2018 • edited Loading

amirfefer left a comment

Choose a reason for hiding this comment

ohadlevy commented May 22, 2018

amirfefer commented May 22, 2018 • edited Loading

Rohoover commented May 22, 2018

ohadlevy commented May 22, 2018 via email

Rohoover commented May 22, 2018

ares commented May 23, 2018

lizagilman commented May 23, 2018 • edited Loading

ares commented May 23, 2018

lizagilman commented Jun 3, 2018

amirfefer left a comment

Choose a reason for hiding this comment

lizagilman commented Jun 5, 2018

boaz0 commented Jun 5, 2018

lizagilman commented Jun 6, 2018

tbrisker left a comment

Choose a reason for hiding this comment

amirfefer left a comment •

edited

Loading

lizagilman commented May 15, 2018 •

edited

Loading

lizagilman commented May 21, 2018 •

edited

Loading

amirfefer commented May 22, 2018 •

edited

Loading

lizagilman commented May 23, 2018 •

edited

Loading