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

Fixes #24011 - Add Patternfly bar chart support #5718

Merged
merged 1 commit into from Nov 5, 2018

Conversation

Projects
None yet
@lizagilman
Copy link
Member

lizagilman commented Jun 20, 2018

To do:

  • fix legend
  • fix labels
  • add tests
  • add to storybook
  • possibly increase the size of the chart ?
  • align chart
  • move to chartbox
  • add tests to wrapper component

edit:

updated screenshot -

image

@theforeman-bot

This comment has been minimized.

Copy link
Member

theforeman-bot commented Jun 20, 2018

Issues: #24011

@lizagilman

This comment has been minimized.

Copy link
Member Author

lizagilman commented Jun 20, 2018

@ripcurld @amirfefer

@lizagilman lizagilman force-pushed the lizagilman:bar-chart branch 2 times, most recently from 2d1f6be to 3a5215a Jun 20, 2018

@amirfefer
Copy link
Member

amirfefer left a comment

Thanks @lizagilman -
I left some inline comments.

What do you think about adding the bar chart functionality to the ChartBox component? then we could use the chartbox component in the metrics page, and by that we keep the consistency from the Statistics page.

donutChartConfig,
donutLargeChartConfig,
barChartConfig,
} from './ChartService.consts';

const sizeConfig = {

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jun 20, 2018

Member

It's not a size configuration anymore, could you rename?

@@ -29,6 +34,7 @@ const getChartConfig = ({
data, config, onclick, id = uuidV1(),
}) => {
const chartConfigForType = sizeConfig[config];

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jun 20, 2018

Member

redundant line

@@ -11,7 +11,8 @@
<div class="col-md-4">
<div class="stats-well">
<h4 class="ca" ><%= _('Report Status') %></h4>
<%= flot_bar_chart("status" ,"", _("Number of Events"), status, :class => "statistics-bar")%>
<div id="status_chart" class='metrics-chart'></div>
<%= mount_react_component('BarChart', '#status_chart', {barChartData: status.to_a, label: "Number of Events"}.to_json) %>

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jun 20, 2018

Member

there's another BarChart in the dashboard (run distribution widget)


data: { ...chartConfig.data, type: 'bar' },

legend: { show: true },

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jun 20, 2018

Member

IMO the legend default should be false
looks odd with only one category

export const barChartConfig = {
...chartConfig,

data: { ...chartConfig.data, type: 'bar' },

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jun 20, 2018

Member

Do we have to add type: 'bar' prop? patternfly BarChart implementation adds it anyway

},
},

categories,

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jun 20, 2018

Member

do we need this here as well?


columns.unshift(label);

chartConfig.data.columns = [columns];

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jun 20, 2018

Member

looks like the data structure doesn't support in groups/stacks

This comment has been minimized.

Copy link
@lizagilman

lizagilman Jul 9, 2018

Author Member

Do you referr to grouped bar chart?
Grouped bar chart expects the following structure: [category1, value1, value2, ...], [category2, value3, value4, ...]
Do we have anywhere data that can be used or manipulated to form this structure?
The 'regular' structure ( (DATA, VALUE) ) is good for the single bar (not grouped) chart, which expects only one value [category, value]

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jul 9, 2018

Member

I'm not sure.

@ohadlevy - would we like to support grouped chart?
grouped

@@ -52,6 +58,7 @@ const getChartConfig = ({
...chartConfigForType,
id,
data: {
...chartConfigForType.data,

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jun 20, 2018

Member

I think if we'll remove {type: bar} from the config object, then we can remove this line.


columns.unshift(label);

chartConfig.data.columns = [columns];

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jun 20, 2018

Member

I have mixed feeling about this. looks like we modify columns property in two different places, if the data is calculated differently from donut chart, maybe it would be better to separate the logic between the two.

This comment has been minimized.

Copy link
@lizagilman

lizagilman Jul 9, 2018

Author Member

I agree, isn't this separation (between the two functions) enough?

data,
onclick,
config = 'regularBar',
noDataMsg = __('No data available'),

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jun 21, 2018

Member

Better to use defaultProps and Proptypes

@amirfefer
Copy link
Member

amirfefer left a comment

thanks @lizagilman
please add yarn.lock to gitignore

@@ -67,6 +67,11 @@ def get_overview_json(report, options = {})

def render_run_distribution(hosts, options = {})
data = count_reports(hosts, options)
puts '*********************************************'

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jul 9, 2018

Member

forgot to delete this

This comment has been minimized.

Copy link
@lizagilman

lizagilman Jul 10, 2018

Author Member

😅

@@ -96,7 +96,7 @@
"uuid": "^3.0.1"
},
"scripts": {
"lint": "./node_modules/.bin/eslint -c .eslintrc webpack/ script/ || exit 0",
"lint": "./node_modules/.bin/eslint -c .eslintrc webpack/ script/",

This comment has been minimized.

Copy link
@amirfefer
@@ -29,7 +29,7 @@ export function templateSelected(item) {
$('#storage_volumes .children_fields >.fields').remove();
$.each(result.volumes, function () {
// Change variable name because 'interface' is a reserved keyword.
this.disk_interface = this['interface'];
this.disk_interface = this.interface;

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jul 9, 2018

Member

well I do find this better syntax, but how does it related?

const BarChart = ({
data,
onclick,
noDataMsg = __('No data available'),

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jul 9, 2018

Member

could it be defined in the defaultProps?

This comment has been minimized.

Copy link
@lizagilman

lizagilman Jul 10, 2018

Author Member

I think it could be done when your jed pr (5342) is merged

BarChart.propTypes = {
config: PropTypes.string,
noDataMsg: PropTypes.string,
title: PropTypes.object,

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jul 9, 2018

Member

please use shape instead of just .object


const categories = data.map(dataItem => dataItem[0]);

const columns = data.map(x => x[1]);

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jul 9, 2018

Member

@sharvit This ChartSevice becomes more and more complex, each render this will invoked, even if nothing changed.
what do you think about moving this logic to a selector (memorized if needed) by connect this component to the store, or its parent.

This comment has been minimized.

Copy link
@sharvit

sharvit Oct 17, 2018

Contributor

Can we actually move it into selectors? I think most of the data coming from the data prop and not from the state.
Does it actually lead to performance issue that we must solve in this particular PR?


columns.unshift(label);

chartConfig.data.columns = [columns];

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jul 9, 2018

Member

I'm not sure.

@ohadlevy - would we like to support grouped chart?
grouped

const configForType = {
regularDonut: donutChartConfig,
largeDonut: donutLargeChartConfig,
regularBar: barChartConfig,

This comment has been minimized.

Copy link
@boaz0

boaz0 Jul 10, 2018

Member

IMHO, since there is only one size (regular) to BarChart, it is not a good idea to add it to the configForType (previously sizeConfig) object.

If we insist on doing that, maybe keep the name sizeConfig and nested it:

const sizeConfig = {
  donut: {
    regular: ...
    large: ...
  },
  bar: {
    ...
  }
}

const getChartConfig = ({
  ...
  type,
  config
}) = {
  chartConfigForType = sizeConfig[type][config]
  ...
}
...chartConfig,
size: barChartEnums.SIZE.REGULAR,

// data: { ...chartConfig.data, },

This comment has been minimized.

Copy link
@boaz0

boaz0 Jul 10, 2018

Member

did you forget to uncomment this? 😃

This comment has been minimized.

Copy link
@lizagilman

lizagilman Jul 10, 2018

Author Member

😅

@lizagilman lizagilman force-pushed the lizagilman:bar-chart branch 5 times, most recently from cb15536 to 2add6cd Jul 10, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 10, 2018

Coverage Status

Coverage increased (+0.2%) to 75.891% when pulling 5b7dc0f on lizagilman:bar-chart into 882d451 on theforeman:develop.

@lizagilman lizagilman force-pushed the lizagilman:bar-chart branch 2 times, most recently from 277152f to d5d16b7 Jul 12, 2018

def render_run_distribution(hosts, options = {})
data = count_reports(hosts, options)
flot_bar_chart("run_distribution", _("Minutes Ago"), _("Number Of Clients"), data, options)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jul 12, 2018

Layout/TrailingWhitespace: Trailing whitespace detected.

@lizagilman lizagilman force-pushed the lizagilman:bar-chart branch from d5d16b7 to 1a3b4a2 Jul 12, 2018

@tbrisker tbrisker added the WIP label Jul 15, 2018

@lizagilman

This comment has been minimized.

Copy link
Member Author

lizagilman commented Jul 15, 2018

@amirfefer @ripcurld
pushed some updates
still need to work on the look of the charts

image

@amirfefer
Copy link
Member

amirfefer left a comment

Thanks @lizagilman, looks great
Could you use ChartBox in the metrics page?


const BarChart = ({
data,
onclick,

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jul 22, 2018

Member

some props missing in the proptype

.gitignore Outdated
@@ -48,3 +48,4 @@ public/webpack
package-lock.json
npm-debug.log
.vscode
yarn.lock

This comment has been minimized.

Copy link
@amirfefer

amirfefer Jul 22, 2018

Member

are you using yarn?

@lizagilman

This comment has been minimized.

Copy link
Member Author

lizagilman commented Oct 26, 2018

[test foreman]

@lizagilman

This comment has been minimized.

Copy link
Member Author

lizagilman commented Oct 28, 2018

@sharvit @amirfefer any other comments here?

@amirfefer
Copy link
Member

amirfefer left a comment

Thanks @lizagilman ! 👍
almost there, few minor issues :)

metricsData: { tableData, tableClasses, total },
} = props.data;

const createRow = (metric, i) => (

This comment has been minimized.

Copy link
@amirfefer

amirfefer Oct 28, 2018

Member

nitpick: please use array destructing here:

const createRow = ([name, value], i) => (
    <tr key={i}>
      <td className="break-me">{name}</td>
      <td>{value}</td>
    </tr>
);
type="donut"
chart={{ data: metricsChartData, id: 'report-metrics' }}
title="Report Metrics"
id="report-metrics"

This comment has been minimized.

Copy link
@amirfefer

amirfefer Oct 28, 2018

Member

I didn't find any use of id prop in Chartbox

@@ -79,7 +94,7 @@ class ChartBox extends React.Component {
<Modal.Title>{this.props.title}</Modal.Title>
</Modal.Header>
<Modal.Body>
<Chart {...chartProps} config="large" />;
<Chart {...chartProps} config={this.props.config} />

This comment has been minimized.

Copy link
@amirfefer

amirfefer Oct 28, 2018

Member

nitpick:
could you extract the props ?

const { config, title, className, status } = this.props
import PropTypes from 'prop-types';
import componentRegistry from '../componentRegistry';

const ComponentWrapper = (props) => {

This comment has been minimized.

Copy link
@amirfefer

amirfefer Oct 28, 2018

Member

index.js meant for redux containers (connected components), I prefer to use componentWrapper.js as a name
Plus, shouldn't it be under common directory?

const { component, componentProps } = props.data;

if (component === 'ComponentWrapper') {
throw new Error('Cannot wrap componenet wrapper');

This comment has been minimized.

Copy link
@amirfefer

amirfefer Oct 28, 2018

Member

could you add a test for this

}),
};

export default ConfigReports;

This comment has been minimized.

Copy link
@amirfefer

amirfefer Oct 28, 2018

Member

same here (use ConfigReport.js instead of index.js)

import { translate as __ } from '../../common/I18n';
import { STATUS } from '../../constants';

const ConfigReports = (props) => {

This comment has been minimized.

Copy link
@amirfefer

amirfefer Oct 28, 2018

Member

could you add a snapshot test for this component ?

@lizagilman

This comment has been minimized.

Copy link
Member Author

lizagilman commented Oct 28, 2018

@amirfefer
updated, thanks!

@lizagilman

This comment has been minimized.

Copy link
Member Author

lizagilman commented Oct 28, 2018

[test katello]

@amirfefer
Copy link
Member

amirfefer left a comment

@lizagilman Works and looks great !
thanks 👍

@lizagilman

This comment has been minimized.

Copy link
Member Author

lizagilman commented Nov 1, 2018

@tstrachota can we merge?

@tstrachota
Copy link
Member

tstrachota left a comment

I found one issue in code. Otherwise it looks good. I still need to test it live though.

@@ -22,6 +22,13 @@ def metric(m)
m.round(4) rescue _("N/A")
end

def metrics_table_data(metrics)
metrics.map do |title, value|

This comment has been minimized.

Copy link
@tstrachota

tstrachota Nov 2, 2018

Member

Result of this method is thrown away.

@lizagilman

This comment has been minimized.

Copy link
Member Author

lizagilman commented Nov 5, 2018

thanks @tstrachota , updated

@tstrachota
Copy link
Member

tstrachota left a comment

I tested this and it works well.

@tstrachota

This comment has been minimized.

Copy link
Member

tstrachota commented Nov 5, 2018

Test failures are unrelated, merging. Thanks @lizagilman !

@tstrachota tstrachota merged commit 711cbdb into theforeman:develop Nov 5, 2018

5 of 8 checks passed

codeclimate 14 issues to fix
Details
foreman Build finished. 36747 tests run, 5 skipped, 12 failed.
Details
katello Build finished. 4073 tests run, 9 skipped, 1 failed.
Details
Hound No violations found. Woof!
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on bar-chart at 76.097%
Details
prprocessor Commit message style is correct
Details
upgrade Build finished. No test results found.
Details
@Laviro

This comment has been minimized.

Copy link
Contributor

Laviro commented Nov 5, 2018

Hey guys, the component wrapper looks really awesome !

though it seem not to work while the component uses Redux
when props that was received from the server
are named the same as props received from the store.
those props are received to the component as undefined.

my guess is that the mapStateToProps runs after the component wrapper call,
and it fetch data which wasn't initiated yet from the store.

I saw that on regular components it works.. so it seems that something happens when involving Redux mapStateToProps.

any tips how to solve this?
thanks

@amirfefer

This comment has been minimized.

Copy link
Member

amirfefer commented Nov 5, 2018

@Laviro
have you used the wrapper on the unconnected component? I don't think we should wrap a connected component with this wrapper. @lizagilman any thoughts?

@Laviro

This comment has been minimized.

Copy link
Contributor

Laviro commented Nov 5, 2018

Tried to pass the unconnected component through componentRegistry and it worked great !
but than the component is not connected to Redux unfortunately..
If it is not the use case of this helper component then sorry for ruining the party 💃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.