feat(Chart): Add Patternfly Chart componenet#103
Conversation
|
Thanks @yaacov :) happy to see you here |
3e6e393 to
7aceb2c
Compare
|
@ohadlevy Thanks :-) where are you ? |
|
cc @nimrodshn |
b29aa03 to
e5f759f
Compare
|
@ohadlevy who can review this ? |
bed0c26 to
c55c6da
Compare
|
Thanks @yaacov ! As we spoke, I think you can upgrade the @priley86 shared this library with me and with @nimrodshn: After reviewing it, I think you can just use this library with the addition of the |
c55c6da to
5301fb4
Compare
src/components/Chart/Chart.js
Outdated
| import React from 'react'; | ||
| import C3Chart from 'react-c3js'; | ||
|
|
||
| const Chart = props => <C3Chart {...props} className="line-chart-pf" />; |
There was a problem hiding this comment.
- Shouldn't the
line-chart-pfbe only existed for line chart?
I guess each chart has its own class but haven't really checked it. - It should be possible to add more classes when using the component. (e.g.
<Chart data={data} className="my-chart-class" />)
I would suggest something like:
import React from 'react';
import C3Chart from 'react-c3js';
import ClassNames from 'classnames';
const Chart = ({ className, ...props }) => {
className = ClassNames(className, {
'line-chart-pf': props.type === 'line'
});
return <C3Chart {...props} className={className} />;
};
export default Chart;bc4bd0b to
a23686d
Compare
|
@yaacov @sharvit this is a great start.. We also have Bar, Donut: I would try to match those as close as possible... We also have a lot of default settings here: Those get used like this for example: I think we can expose those as default objects/functions and allow consumer to use them by default or override them... we should show this in the storybook stories I am guessing.. Utilization Bar is not using c3... so I think we can ignore that one in this PR. I will try to check in and help review this some more tomorrow... cc: @jeff-phillips-18 what else on the charts? |
src/components/Chart/Chart.js
Outdated
| import ClassNames from 'classnames'; | ||
|
|
||
| const Chart = ({ ...props }) => { | ||
| const chartType = props.data.type || 'line'; |
There was a problem hiding this comment.
I would almost rather throw new Error(`Unsupported type=${type}`); but that is just personal preference. When consuming downstream, if I don't see the type specified some place I will get confused...
a23686d to
f22423f
Compare
|
@priley86 @sharvit thanks for the reviews,
👍 added to the story.
I do not know how to consume [1] https://github.com/patternfly/patternfly/blob/master/src/js/patternfly-settings-charts.js |
|
My preference is to avoid consuming PF jquery in this project if possible (as we have not done that up to this point), but will let @jeff-phillips-18 weigh in on this. I think this is a good question to consider at this point as I see a few good options to sharing these sort of things:
I think there is some clarification needed first... |
f22423f to
d3b60f6
Compare
|
@priley86 👍 Thanks |
|
@yaacov @jeff-phillips-18 I did a bit more homework just now... ;) It looks like patternfly-ng is consuming globals in I think this is a decent alternative - we should be able to add this import into config.js @jeff-phillips-18 does this work for you? |
| const pfSetDonutChartTitle = patternfly.pfSetDonutChartTitle; | ||
|
|
||
| const addDonutTitle = lifecycle({ | ||
| componentDidMount() { |
There was a problem hiding this comment.
two comments:
- should this be only on mount, maybe on update as well?
- maybe it make sense to test the props before calling the pf function (as i assume it would be faster not to call it if its not needed).
There was a problem hiding this comment.
👍 Thanks.
Set also the 'componentDidUpdate' method.
Testing for change is a little tricky and may take the same time as re draw ... need to think how to do that ...
|
/cc @lizagilman |
ea0ae9d to
a3a9625
Compare
src/components/Chart/constants.js
Outdated
| } | ||
|
|
||
| // Set Bar Chart tooltip | ||
| console.log(name, props); |
There was a problem hiding this comment.
Ahhh ... fixed thanks :-)
|
this looks beautiful to me @yaacov 🎨 great job iterating so fast on this. |
a3a9625 to
90a25c2
Compare
| */ | ||
|
|
||
| const donutConfigData = { | ||
| columns: [['MHz Used', 60], ['MHz Available', 40]], |
There was a problem hiding this comment.
This seems to be rendering "60% Mhz" in the title... and I think it should be "60 Mhz".
There was a problem hiding this comment.
fixed :-)
p.s.
Patternfly is very opinionated about that :-)
https://github.com/patternfly/patternfly/blob/master/src/js/patternfly-settings-charts.js#L24
|
@yaacov I don't see any other issue at this point. Terrific job... @jeff-phillips-18 @jgiardino what else? |
|
Tests?
On Dec 13, 2017 8:18 PM, "Patrick Riley" <notifications@github.com> wrote:
@yaacov <https://github.com/yaacov> I don't see any other issue at this
point. Terrific job...
@jeff-phillips-18 <https://github.com/jeff-phillips-18> @jgiardino
<https://github.com/jgiardino> what else?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#103 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABOx-EEVeJtqrVR3hweOpZCZ0zvUMhsks5tABULgaJpZM4Q7vxy>
.
|
8ca7d6d to
3d2499f
Compare
|
These are great! The only thing I notice is that there is a bounce effect in the legend of the pie chart, and there isn't one on the donut. But when I check patternfly, I see some inconsistency there. For example, the pie chart test page includes the bounce effect, but the code sample on patternfly.org does not. @mcarrano @LHinson Do you know if we have any details on what's expected here? @yaacov Do you know where this setting gets enabled? I don't see this as something that should block merging this PR, though. We can capture it and revisit it later. |
There was a problem hiding this comment.
Other than wanting to know where the bounce animation in the legend is coming from, these look good to me.
|
@jgiardino hi,
This is what I have on fedora27 default filrefox, it looks the same to me, what am I missing ?
|
|
I don't see the effect in Firefox either. After poking around, I don't think it's an intentional effect. When I disable the following style, it doesn't happen: So it looks like this is just a side effect of having crisp edges and altering the opacity, which isn't something we have control over. |
|
cool! great job @yaacov! great job React team ;) |
|
Nice!! I love those changes! |
use localhost i 9D76 nstead of 0.0.0.0 for host so app opens correctly on windows use enzyme-adapter-react-17 to ensure jest test works remove stub async module support prevent url from changing when selecting SkipToContent component update readme screenshot preview patch storybook
What:
Add a chart component.
Why:
We currently use line charts to display data using angular [1], we want the option to render the same charts using react.
How:
This implementation is copy-paste react component of the Patternfly example here [2].
Storybook:
https://yaacov.github.io/patternfly-react/?knob-Label=Danger%20Will%20Robinson%21&selectedKind=Chart&selectedStory=Basic%20Example&full=0&down=1&left=1&panelRight=0&downPanel=storybooks%2Fstorybook-addon-knobs
[1] https://github.com/patternfly/angular-patternfly/tree/master/src/charts/line
[2] http://www.patternfly.org/pattern-library/data-visualization/line-chart/#/code