feat(charts): add remaining variables #1863

christiemolloy · 2019-05-28T19:53:37Z

This Pull Request is for the remaining variables for Charts. I spoke to @dlabrecq and these vars should be specific to the chart variations which I followed here . @gdoyle1 could you review to check that I properly covered the variables here ?

patternfly-build · 2019-05-28T19:56:32Z

Deploy preview for pf-next ready!

Built with commit 1951dbb

https://deploy-preview-1863--pf-next.netlify.com

gdoyle1 · 2019-05-28T20:00:26Z

Looks good! @christiemolloy

christiemolloy · 2019-05-28T19:57:20Z

src/patternfly/patternfly-charts.scss

+$pf-chart-chart--BorderWidth: 1px;
+
+// Donut Chart
+$pf-chart-donut-color-threshold-large: $pf-color-black-200;


@dlabrecq donut colors weren't on theme-victory.js do you want them included here?

christiemolloy · 2019-05-28T19:57:59Z

src/patternfly/patternfly-charts.scss

+$pf-chart-boxplot-box--Width: 20px;
+
+// Bullet Chart
+$pf-chart-bullet--comparative-measure-line-warning--BorderColor: $pf-color-orange-300;


@dlabrecq bullet chart colors and Borders weren't on theme-victory.js do you want them included here?

Yes, please.

We will have a separate theme files for bullet chart, donut, and the new donut utilization charts below. However, we still need the vars to use with the components.

The props for the donut chart can be found here:
https://github.com/patternfly/patternfly-react/blob/master/packages/patternfly-4/react-charts/src/components/ChartTheme/themes/theme-donut.js

The props for the donut utilization chart are here:
https://github.com/patternfly/patternfly-react/blob/master/packages/patternfly-4/react-charts/src/components/ChartTheme/themes/theme-donut-utilization.js

The props for the new donut utilization chart with static thresholds are here:
https://github.com/patternfly/patternfly-react/blob/master/packages/patternfly-4/react-charts/src/components/ChartTheme/themes/theme-donut-threshold.js

@dlabrecq
Updated based on these files. Some questions.

In pie chart, what is padAngle?

Those font sizes should be 14 and 24 instead of 16 and 26, so they relate to the global font sizes

Do you need a variable for: orientation: vertical?

inside of the donut key value pairs, is "pie". Do those variables relate to pie or donut? I've seen the same height/width of 230 be used under pie but in the pie variation and in the pie inside of donut.

padAngle is the space between the pie (i.e., dount) chart segments. This is an example with padAngle=1, which looks like 3 pixels in the design.

pie and donut are separate components, but Victory only supports pie charts. When creating a donut theme, we merge the donut properties on top of the pie properties. That gives us the look of the donut, but it's actually a pie chart.

The legend component has a default orientation, so probably not necessary?

That makes sense about pie and donut, and since only colors are defined for donut it keeps the properties of the pie separate.

Also I see in the material.js theme that the legend has an orientation of 'vertical' which is different to the victory file where the legend has an orientation of 'horizontal', so i think ill keep it in there for now?

christiemolloy · 2019-05-28T19:58:56Z

src/patternfly/patternfly-charts.scss

+$pf-chart-bar--Width: 10px;
+
+// Box Plot Chart
+$pf-chart-boxplot-max--Padding: $pf-global--spacer--sm;


@mcoker I repeat alot of variables that have the same value here, just want to be cautious in case they change what are your thoughts?

I'll defer to @mcoker, but if we have separate vars for each component, then devs can override properties for just that one chart component. That is, without changing properties for all charts.

@dlabrecq agreed.

src/patternfly/patternfly-charts.scss

christiemolloy · 2019-05-28T20:00:39Z

src/patternfly/patternfly-charts.scss

+
+
+// typography
+$pf-chart-global--FontSize: $pf-global--FontSize--sm;


@mcoker do I need to pull in the Red Hat Font to these style sheet?

@mcoker not sure if you missed this comment :)

christiemolloy · 2019-05-28T20:01:25Z

src/patternfly/patternfly-charts.scss

+
+// Area Chart
+$pf-chart-area--Opacity: .4;
+$pf-chart-area-stroke--Width: 2px;


@mcoker im not sure whether to break stroke width into two words or not

I think this is fine, though I think we should follow the same general formula we do for other global vars and use $pf-chart-area--stroke--Width. Though what is the "stroke" width? Is that the same as a border?

Thats the thing I was debating. @dlabrecq do you know if stroke width is different from border width, if so @mcoker I thought it was more of a concept rather than a property.

i.e. here there is a stroke width and a border width?

Yes, they are different.

christiemolloy · 2019-05-28T20:02:33Z

src/patternfly/patternfly-charts.scss

+$pf-chart-candelstick-data-stroke--Width: 1px;
+
+// Chart
+$pf-chart-chart--BorderWidth: 1px;


Should the "chart" variation of charts be defined globally? or is it actually a variation @dlabrecq

Chart is a separate component. Basically it's a container which can be used with any chart to provide default functionality like a border, x and y axes, etc.

FYI, the border is currently border: 1px solid. Do we have solid defined anywhere?

@dlabrecq when we usually define vars we wouldn't do one for solid, but this makes sense since its the only css defined for use in the victory charts

They could also be separate vars and we concatinate them in the Victory theme?

@mcoker what do you think? borders by default are solid right. Across the system I see alot of border widths without the 'solid'. I'm not sure if the solid is necessary.

The only border I see in the Victory themes is for the Chart component -- not including borderWidth props. Keep in mind that these are SVGs, not CSS props.

christiemolloy · 2019-05-28T20:02:57Z

src/patternfly/patternfly-charts.scss

+
+// Donut Chart
+$pf-chart-donut-color-threshold-large: $pf-color-black-200;
+$pf-chart-donut-color-threshold-medium: $pf-color-black-300;


@gdoyle1 does small/med/large work here for naming?

first/second/third thresholds

gdoyle1 · 2019-05-28T21:25:09Z

src/patternfly/patternfly-charts.scss

+
+
+// typography
+$pf-chart-global--FontSize: $pf-global--FontSize--sm;


axis labels and scale values should be 12px

for donut charts with the a single label in the middle representing data value, should be 24px

legend should be using 14px text

dlabrecq

This is a good start. We need a few more props, tho.

Note that we cannot consume rem props, like the global spacers -- must be in pixels.

Likewise, we cannot consume props defined with px strings. For example, 100px must be 100.

If we must define vars as 100px, we will need to discuss how the charts will consume that.

src/patternfly/patternfly-charts.scss

dlabrecq · 2019-05-29T15:04:56Z

src/patternfly/patternfly-charts.scss

+// typography
+$pf-chart-global--FontSize--xs: $pf-global--FontSize--xs; // Axis Labels
+$pf-chart-global--FontSize--sm: $pf-global--FontSize--sm; // For Legend
+$pf-chart-global--FontSize--2xl: $pf-global--FontSize--2xl; // For Large Numbers


Do we want to have a var for the chart font family? This is currently hard coded as global_FontFamily_sans_serif.value

I tagged @mcoker about this as I'm not sure the best way to import the new font family

I think so. Seems like we would want the chart theme to only use $pf-chart vars? Then it allows us the flexibility in the future to deviate from core by simply updating the variable value, instead of also needing to update the react chart theme code.

dlabrecq · 2019-05-29T15:13:57Z

src/patternfly/patternfly-charts.scss

+$pf-chart-boxplot-box--Width: 20px;
+
+// Bullet Chart
+$pf-chart-bullet--comparative-measure-line-warning--BorderColor: $pf-color-orange-300;


Yes, please.

We will have a separate theme files for bullet chart, donut, and the new donut utilization charts below. However, we still need the vars to use with the components.

The props for the donut chart can be found here:
https://github.com/patternfly/patternfly-react/blob/master/packages/patternfly-4/react-charts/src/components/ChartTheme/themes/theme-donut.js

The props for the donut utilization chart are here:
https://github.com/patternfly/patternfly-react/blob/master/packages/patternfly-4/react-charts/src/components/ChartTheme/themes/theme-donut-utilization.js

The props for the new donut utilization chart with static thresholds are here:
https://github.com/patternfly/patternfly-react/blob/master/packages/patternfly-4/react-charts/src/components/ChartTheme/themes/theme-donut-threshold.js

dlabrecq · 2019-05-29T15:15:59Z

src/patternfly/patternfly-charts.scss

+$pf-chart-candelstick-data-stroke--Width: 1px;
+
+// Chart
+$pf-chart-chart--BorderWidth: 1px;


Chart is a separate component. Basically it's a container which can be used with any chart to provide default functionality like a border, x and y axes, etc.

dlabrecq · 2019-05-29T15:16:55Z

src/patternfly/patternfly-charts.scss

+$pf-chart-candelstick-data-stroke--Width: 1px;
+
+// Chart
+$pf-chart-chart--BorderWidth: 1px;


FYI, the border is currently border: 1px solid. Do we have solid defined anywhere?

dlabrecq · 2019-05-29T15:42:53Z

src/patternfly/patternfly-charts.scss

+
+// Line Chart
+$pf-chart-line-stroke--Width: 2px;
+


I noticed that there are some properties that are not defined in victory-theme.js. Apparently, we're getting some defaults by relying on the Victory grayscale theme. However, we probably want to add our own vars. For example, line chart can have these props:

Victory has a grayscale.js theme, which shows the other available props. For example, line can have fill, stroke, strokeWidth, etc.
https://github.com/FormidableLabs/victory/blob/master/packages/victory-core/src/victory-theme/grayscale.js

Victory also has a material.js theme which uses some props that are not listed in grayscale.js. For example, this theme defines an opacity for line chart.
https://github.com/FormidableLabs/victory/blob/master/packages/victory-core/src/victory-theme/material.js
https://github.com/FormidableLabs/victory/blob/master/packages/victory-core/src/victory-theme/material.js

Added in the remaining properties from both of those files

dlabrecq · 2019-05-29T15:45:42Z

src/patternfly/patternfly-charts.scss

+
+// Pie Chart
+$pf-chart-pie--Padding: $pf-global--spacer--sm;
+$pf-chart-pie-data-Padding: $pf-global--spacer--sm;


I don't believe we can use rem values here. This must be pixels. For example, the donut relies on this being 8px.

dlabrecq · 2019-05-29T15:47:18Z

src/patternfly/patternfly-charts.scss

+$pf-chart-pie--Width: 230px;
+
+// Scatter Chart
+$pf-chart-stack-data-stroke--Width: 1px;


The scatter chart currently has strokeWidth set to 0 and stroke as transparent

dlabrecq · 2019-05-29T15:49:09Z

src/patternfly/patternfly-charts.scss

+// Tooltip
+$pf-chart-tooltip-pointer--Height: 10px; // this is equivalent to length
+$pf-chart-tooltip-pointer--Width: 20px;
+$pf-chart-tooltip--Padding: $pf-global--spacer--md;


The tooltip has a bunch of important props that effect the styling. For example, cornerRadius, strokeWidth, padding, etc.

dlabrecq · 2019-05-29T15:51:10Z

src/patternfly/patternfly-charts.scss

+
+// Voronoi Chart
+$pf-chart-voronoi-label--Padding: $pf-global--spacer--sm;
+$pf-chart-voronoi-flyout-stroke--Width: 1px;


Missing a few important props here. For example, padding, strokeWdith, etc., which are different than the flyout tooltip styles

mcoker · 2019-05-29T18:26:09Z

src/patternfly/patternfly-charts.scss

+$pf-chart-area-stroke--Width: 2px;
+
+// Axis Chart
+$pf-chart-axis-axis-stroke--Width: 1px;


why is axis in the var name twice?

It refers to the axis within the axis variation of the chart, it was for consistency:

mcoker · 2019-05-29T18:28:51Z

src/patternfly/patternfly-charts.scss

+
+// Box Plot Chart
+$pf-chart-boxplot-max--Padding: $pf-global--spacer--sm;
+$pf-chart-boxplot-max-stroke--Width: 1px;


Similar to my other comment https://github.com/patternfly/patternfly-next/pull/1863/files#r288707836

I think the prefix should be $pf-chart-global or $pf-chart-boxplot followed by --concept (or thing you're styling) followed by --modifier (if applicable), followed by --PropertyCamelCase

So this would become

$pf-chart-boxplot--max--stroke--Width
$pf-chart-boxplot--median--stroke--Width
$pf-chart-boxplot--min--stroke--Width
etc...

Tried to follow this throughout the doc, but it would be great if you could double check the changes

mcoker · 2019-05-30T19:51:20Z

src/patternfly/patternfly-charts.scss

+$pf-chart-boxplot-min--Padding: $pf-global--spacer--sm;
+$pf-chart-boxplot-min-stroke--Width: 1px;
+$pf-chart-boxplot-q1--Padding: $pf-global--spacer--sm;
+$pf-chart-boxplot-q3--Padding: $pf-global--spacer--sm;


what do q1 and q3 represent?

These are Victory BoxPlot component properties. You can read about them here: https://formidable.com/open-source/victory/docs/victory-box-plot#q1

mcoker · 2019-05-30T19:52:19Z

src/patternfly/patternfly-charts.scss

+
+// Bullet Chart
+$pf-chart-bullet--comparative-measure-line-warning--BorderColor: $pf-color-orange-300;
+$pf-chart-bullet--comparative-measure-line-warning--BorderWidth: 1px;


in this line and the ones below "warning" and "error" are states, so there should be 2 hyphens before that text.

mcoker · 2019-05-30T19:52:59Z

src/patternfly/patternfly-charts.scss

+$pf-chart-bullet--negative-primary-measure--BorderWidth: 2px;
+
+// Candlestick
+$pf-chart-candelstick-data-stroke--Width: 1px;


should this be $pf-chart-candelstick--data--stroke--Width?

mcoker · 2019-05-30T19:53:49Z

src/patternfly/patternfly-charts.scss

+$pf-chart-chart--BorderWidth: 1px;
+
+// Donut Chart
+$pf-chart-donut-color-threshold-first: $pf-color-black-200;


Should this be $pf-chart-donut--threshold--first--Color?

mcoker · 2019-05-30T20:02:01Z

src/patternfly/patternfly-charts.scss

+$pf-chart-tooltip--pointerLength: 10; // grayscale theme
+$pf-chart-tooltip--flyoutStyle--cornerRadius: 0;
+$pf-chart-tooltip--flyoutStyle--stroke--Width: 0; // this is 1 in grayscale theme
+$pf-chart-tooltip--flyoutStyle--pointerEvents: "none";


pointer-events is a property name, so you could call this PointerEvents

mcoker · 2019-05-30T20:02:55Z

src/patternfly/patternfly-charts.scss

+// Tooltip
+$pf-chart-tooltip--cornerRadius: 0; // this is 5 in grayscale theme
+$pf-chart-tooltip--pointerLength: 10; // grayscale theme
+$pf-chart-tooltip--flyoutStyle--cornerRadius: 0;


can we just hyphenate names that aren't CSS properties? so this would be corner-radius. Also could we use BorderRadius here?

Updated all names that aren't CSS properties to have hyphens.

@dlabrecq do you know if corner radius and border radius are the same things?

corner radius is different than border. For example, we want the cornerRadius=0 so our tooltip boxes look be square.

mcoker · 2019-05-30T20:03:43Z

src/patternfly/patternfly-charts.scss

+$pf-chart-tooltip--pointer--Height: 10; // this is equivalent to length
+$pf-chart-tooltip--pointer--Width: 20;
+$pf-chart-tooltip--Padding: 16;
+$pf-chart-tooltip--pointerEvents: "none";


Suggested change

$pf-chart-tooltip--pointerEvents: "none";

$pf-chart-tooltip--PointerEvents: "none";

mcoker · 2019-05-30T20:03:53Z

src/patternfly/patternfly-charts.scss

+$pf-chart-tooltip--Padding: 16;
+$pf-chart-tooltip--pointerEvents: "none";
+$pf-chart-tooltip--labels--Padding: 5; // grayscale theme
+$pf-chart-tooltip--labels--pointerEvents: "none"; // grayscale theme


Suggested change

$pf-chart-tooltip--labels--pointerEvents: "none"; // grayscale theme

$pf-chart-tooltip--labels--PointerEvents: "none"; // grayscale theme

mcoker · 2019-05-30T20:04:58Z

src/patternfly/patternfly-charts.scss

+$pf-chart-voronoi--data--stroke--Color: transparent;
+$pf-chart-voronoi--data--stroke--Width: 0;
+$pf-chart-voronoi--labels--Padding: 8; // this is 5 in grayscale theme
+$pf-chart-voronoi--labels--pointerEvents: "none";


Suggested change

$pf-chart-voronoi--labels--pointerEvents: "none";

$pf-chart-voronoi--labels--PointerEvents: "none";

mcoker · 2019-05-30T20:05:06Z

src/patternfly/patternfly-charts.scss

+$pf-chart-voronoi--labels--Padding: 8; // this is 5 in grayscale theme
+$pf-chart-voronoi--labels--pointerEvents: "none";
+$pf-chart-voronoi--flyout--stroke--Width: 1;
+$pf-chart-voronoi--flyout--pointerEvents: "none";


Suggested change

$pf-chart-voronoi--flyout--pointerEvents: "none";

$pf-chart-voronoi--flyout--PointerEvents: "none";

mcoker · 2019-05-30T20:05:31Z

src/patternfly/patternfly-charts.scss

+$pf-chart-voronoi--flyout--pointerEvents: "none";
+$pf-chart-voronoi--flyout--stroke--Color: "charcoal";
+$pf-chart-voronoi--flyout--stroke--BackgroundColor: #f0f0f0;
+$pf-chart-voronoi--flyout--pointerEvents: "none";


Suggested change

$pf-chart-voronoi--flyout--pointerEvents: "none";

$pf-chart-voronoi--flyout--PointerEvents: "none";

mcoker · 2019-05-30T20:19:45Z

src/patternfly/patternfly-charts.scss

+$pf-chart-axis--grid--BackgroundColor: transparent; // is transparent ok for none?
+$pf-chart-axis--grid--stroke--Color: "blueGrey50"; // material theme
+$pf-chart-axis--grid--BorderColor: transparent;
+$pf-chart-axis--grid--pointerEvents: "painted";


Suggested change

$pf-chart-axis--grid--pointerEvents: "painted";

$pf-chart-axis--grid--PointerEvents: "painted";

christiemolloy · 2019-05-30T20:24:56Z

@gdoyle1 there are two themes , one called grayscale and one called material. Right now we are relying on them so we want to use the same props that they include. Could you take a look at: https://github.com/FormidableLabs/victory/blob/master/packages/victory-core/src/victory-theme/grayscale.js and when you see the assignments at the top of the file could you take a look at these colors and let me know what they "should" represent in our color system.

there are 7 colors in the 'color' array --> what should these represent, the grey chart family?
what should 'charcoal' represent?
what should 'grey' represent?

I believe we will also need a conversion for the Material.js theme :) https://github.com/FormidableLabs/victory/blob/master/packages/victory-core/src/victory-theme/material.js

mcoker · 2019-05-30T20:36:20Z

src/patternfly/patternfly-charts.scss

+$pf-chart-boxplot--min--Padding: 8;
+$pf-chart-boxplot--min--stroke--Width: 1;
+$pf-chart-boxplot--min--stroke--Color: "charcoal"; // grayscale theme
+$pf-chart-boxplot--q1--Padding: 8;


Lost the previous comment. @dlabrecq you pointed me here but I'm still not sure what q1 is :) I'm just curious if we can give a more accessible name/description for that var instead of something that's victory specific.

q1 means Lower Quartile and q3 means Upper Quartile, basically the beginning and end of the median.

gdoyle1 · 2019-05-31T19:15:42Z

@christiemolloy Regarding the material color replacements, see here!

#F5C12E should be #F0AB00

mcoker · 2019-06-17T16:09:31Z

@dlabrecq I'd like to review it again. I'll do that after lunch today.

mcoker · 2019-06-17T18:29:34Z

src/patternfly/components/Nav/examples/index.js

@@ -30,7 +30,7 @@ import docs from '../docs/code.md';

 export const Docs = docs;

-export default (props) => {
+export default props => {


did you mean to commit this?

@redallen do you know if the parentheses are necessary? Looks like every component/layout has them except for

marvin:patternfly-next cmichael$ grep -ri 'export default' src/ | grep props | grep -v '(props)' src//patternfly/components/Alert/examples/index.js:export default props => { src//patternfly/components/AppLauncher/examples/index.js:export default props => { src//patternfly/components/NotificationBadge/examples/index.js:export default props => { src//patternfly/components/AlertGroup/examples/index.js:export default props => {

mcoker · 2019-06-17T18:58:25Z

we use danger and error in the var names, and values for each in the other's namespace - should we normalize and use one or the other or do they represent different things?
should we have vars for stroke/border width? (1 & 2 mostly it seems)
should we have vars for padding/gutter (or just spacer like core)? (1, 8, 10, 16, 20, maybe 50?)
- I don't want to get us into a situation where if we introduce a new var, it messes up the naming. For example, if $spacer-1: 1; $spacer-2: 8; and we add a new one as "6", what var do we call it? Do we have the concept of a "spacer system" here?
we use "230" for pie height/width in multiple spots - should that be a var?
does $pf-chart-global--layout--Height and $pf-chart-global--layout--Width need to be a var since we don't define height/width anywhere else (except maybe pie above)? Or should we create a var for all of the chart width/heights if they apply?
in this block below, we don't normally name the concept var by the color var it represents. That kinda defeats the purpose - if we change one of the "fill" vars to a different color, we wouldn't want to have to rename the fill var. Do we have a fill (looks like this is also used for stroke) color system, where we would have Fill--Color--100, Fill--Color--200, etc, similar to how we have other vars defined like that (ie, $pf-chart-global--warning--Color--100, $pf-chart-global--warning--Color--200, etc)

$pf-chart-global--Fill--Color--900: $pf-color-black-900;
$pf-chart-global--Fill--Color--700: $pf-color-black-700;
$pf-chart-global--Fill--Color--500: $pf-color-black-500;
$pf-chart-global--Fill--Color--400: $pf-color-black-400;
$pf-chart-global--Fill--Color--300: $pf-color-black-300;
$pf-chart-global--Fill--Color--200: $pf-color-black-200;
$pf-chart-global--Fill--Color--white: $pf-color-white;

christiemolloy · 2019-06-17T20:09:39Z

@dlabrecq could you provide some insight here. The layout vars are used in alot of charts right so they should stay global? or could it be moved to under the globals like the legend/axis? I think what @mcoker is saying is that we declare it at the top of the file but its not used in each chart type

does $pf-chart-global--layout--Height and $pf-chart-global--layout--Width need to be a var since we don't define height/width anywhere else (except maybe pie above)? Or should we create a var for all of the chart width/heights if they apply?

christiemolloy · 2019-06-17T20:26:49Z

@mcoker I hadn't done that yet because I wasn't sure how extensive the system will get. @gdoyle1 @dlabrecq do you know if we expect to add more colors to these fill colors or is this set ?

in this block below, we don't normally name the concept var by the color var it represents. That kinda defeats the purpose - if we change one of the "fill" vars to a different color, we wouldn't want to have to rename the fill var. Do we have a fill (looks like this is also used for stroke) color system, where we would have Fill--Color--100, Fill--Color--200, etc, similar to how we have other vars defined like that (ie, $pf-chart-global--warning--Color--100, $pf-chart-global--warning--Color--200, etc)

$pf-chart-global--Fill--Color--900: $pf-color-black-900;
$pf-chart-global--Fill--Color--700: $pf-color-black-700;
$pf-chart-global--Fill--Color--500: $pf-color-black-500;
$pf-chart-global--Fill--Color--400: $pf-color-black-400;
$pf-chart-global--Fill--Color--300: $pf-color-black-300;
$pf-chart-global--Fill--Color--200: $pf-color-black-200;
$pf-chart-global--Fill--Color--white: $pf-color-white;

christiemolloy · 2019-06-17T20:27:58Z

@mcoker I don't think 230 should be a variable because pies/donuts overlap anyway so that seems clear .

christiemolloy · 2019-06-17T20:43:47Z

@dlabrecq @gdoyle1 what do you think about using a spacer system here for padding? The values in the charts seem very much component specific, I can't see the use of a global spacer system here really. IMO I see a spacer system using a value set that has a pattern to how it increments.

should we have vars for padding/gutter (or just spacer like core)? (1, 8, 10, 16, 20, maybe 50?)

I don't want to get us into a situation where if we introduce a new var, it messes up the naming. For example, if $spacer-1: 1; $spacer-2: 8; and we add a new one as "6", what var do we call it? Do we have the concept of a "spacer system" here?

christiemolloy · 2019-06-17T20:44:48Z

@gdoyle1 @dlabrecq do you know if error and danger are the same thing?

mcoker · 2019-06-17T20:53:36Z

The layout vars are used in alot of charts right so they should stay global?

Ah ok that makes sense.

I don't think 230 should be a variable because pies/donuts overlap anyway so that seems clear .

I just mean that the width/height 230 pair is defined in 4 spots, so seems easier to maintain if that's a common var instead of hardcoded in each place, as long as we want those to display consistently.

For example, this...

$pf-chart-donut--pie--Height: 230;
$pf-chart-donut--pie--Width: 230;
$pf-chart-donut--threshold--static--pie--Height: 230;
$pf-chart-donut--threshold--static--pie--Width: 230;
$pf-chart-donut--utilization--dynamic--pie--Height: 230;
$pf-chart-donut--utilization--dynamic--pie--Width: 230;
$pf-chart-pie--Height: 230;
$pf-chart-pie--Width: 230;

could become....

$pf-chart-donut--pie--Height: 230;
$pf-chart-donut--pie--Width: 230;
$pf-chart-donut--threshold--static--pie--Height: $pf-chart-donut--pie--Height;
$pf-chart-donut--threshold--static--pie--Width: $pf-chart-donut--pie--Width;
$pf-chart-donut--utilization--dynamic--pie--Height: $pf-chart-donut--pie--Height
$pf-chart-donut--utilization--dynamic--pie--Width: $pf-chart-donut--pie--Width;
$pf-chart-pie--Height: $pf-chart-donut--pie--Height;
$pf-chart-pie--Width: $pf-chart-donut--pie--Width;

gdoyle1 · 2019-06-17T22:49:29Z

@christiemolloy Danger and error are the same, yes - I think more products are using "error" now instead of danger. I think a spacer system would be a good thing to look into making for charts. @mceledonia did you picture using the same number system as we have for our other spacers or a different incremental order? I'm not 100% sure about the colors thing although I don't see more colors being added to the chart colors. @mceledonia would you agree?

mceledonia · 2019-06-18T13:01:43Z

@gdoyle1 yeah I think we can use the same spacer system. I used it when designing the charts, but I have a feeling it will really only come into play for padding of things like axis labels and such. Whether or not it makes sense on the dev side to do that, I'm not confident enough to give an answer there..

dlabrecq · 2019-06-18T13:14:30Z

The 230px value is common to donut and donut utilization and donut threshold. However, they are separate components. I don't know that they should share a global variable simply because there is a similar value? Ideally, all components should have their own vars, but will defer to @mcoker.

dlabrecq · 2019-06-18T13:25:26Z

It's my understanding that the layout vars (font, family, etc.) are global because they are expected to remain consistent for all. Although, they're not necessarily used by all components; for example, legend, pie, tooltip -- maybe deeper in the style props? It's your call as to whether these particular vars are global.

I don't feel strongly about the layout props, but padding may be specific to each component. In that case, we could override.

I don't know if using a global spacer system here is best for charts. Although, perhaps we can speak more about what that means?

We have specific 50px padding for the chart axis, while bar width is 10px. The legend also has a gutter of 20px, which probably should be 10px, IMO. Other than that, we have a few places where we use a padding of 8px (label margin) and 16px (legend margin). Forcing all that to fit in our spacer system may not be ideal?

I'd be leery of changing any properties unless we see what it looks like in the charts, first. Due to the scale of the SVG, you may not get an exact pixel match (e.g., padAngle=1 looks a lot wider when rendered).

dlabrecq · 2019-06-18T14:12:28Z

$pf-chart-global--layout--Height and $pf-chart-global--layout--Width apply to all charts as a default height and width. However, donuts don't need to be that large, so we override that.

I would like to continue with separate vars for padding/gutter and stroke/border width. Although they may share a similar value, these properties have very different effects on charts -- they are not the same.

christiemolloy · 2019-06-18T18:17:24Z

If error and danger are the same do I need to make a new global variable for error?
I added vars for stroke and border width but kept them separate
I agree with @dlabrecq when he said that we shouldn't change the spacers until he tests it in React

@mcoker what do you think

mcoker · 2019-06-19T15:54:37Z

If error and danger are the same do I need to make a new global variable for error?

I think what you have now is fine. We use danger in the global color var names, which matches core. And I'm assuming the use of danger and error in the chart specific vars matches the theme properties.

I added vars for stroke and border width but kept them separate

Did you push those changes up? I don't see them.

I agree with @dlabrecq when he said that we shouldn't change the spacers until he tests it in React

👍

christiemolloy · 2019-06-20T13:44:40Z

@mcoker here is the change: 9ee4958 I must not have pushed it up

mcoker · 2019-06-20T15:12:21Z

@christiemolloy lgtm!

Looks like you missed one:

$pf-chart-area--stroke--Width: 2;

And I'm not sure what this is - would it apply as a stroke/border, too? Or is it something else?

$pf-chart-bullet--threshold-error--Width: 2;

christiemolloy · 2019-06-20T17:12:56Z

Updated! $pf-chart-bullet--threshold-error--Width: 2; is just a width not a stroke or border.

mcoker

Great work on this!!! 🎉

christiemolloy · 2019-06-20T18:41:27Z

@dlabrecq and @mcoker can you re-approve this PR, I just added the files from _chart-globals.scss to patternfly-charts.scss

mcoker · 2019-06-20T21:36:02Z

src/patternfly/patternfly-charts.scss

+  --pf-chart-line--data--Fill: #{$pf-chart-line--data--Fill};
+  --pf-chart-line--data--Opacity: #{$pf-chart-line--data--Opacity};
+  --pf-chart-line--data--stroke--Width: #{$pf-chart-line--data--stroke--Width};
+  --pf-chart-line--data--stroke--Color: #{$pf-chart-global--Fill--Color--900};


Suggested change

--pf-chart-line--data--stroke--Color: #{$pf-chart-global--Fill--Color--900};

--pf-chart-line--data--stroke--Color: #{$pf-chart-line--data--stroke--Color};

mcoker

👍

patternfly-build · 2019-06-21T14:10:41Z

🎉 This PR is included in version 2.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

christiemolloy added 2 commits May 28, 2019 15:20

feat(charts): add variables other than colors

dc72e86

feat(charts): update vars

4e5d006

christiemolloy requested review from mcoker, dlabrecq and gdoyle1 May 28, 2019 19:56

gdoyle1 approved these changes May 28, 2019

View reviewed changes

christiemolloy commented May 28, 2019

View reviewed changes

gdoyle1 reviewed May 28, 2019

View reviewed changes

feat(charts): ginas review

288be15

dlabrecq requested changes May 29, 2019

View reviewed changes

mcoker reviewed May 29, 2019

View reviewed changes

fix(charts): changes based on Dans review

f868d02

mcoker reviewed May 30, 2019

View reviewed changes

mcoker reviewed Jun 17, 2019

View reviewed changes

fix(charts): updates

9ee4958

fix(charts): add remaining var

010648f

mcoker approved these changes Jun 20, 2019

View reviewed changes

fix(charts): add vars to patternfly-charts

b0cbf2f

mcoker reviewed Jun 20, 2019

View reviewed changes

fix(charts): fix mistake

1951dbb

mcoker approved these changes Jun 21, 2019

View reviewed changes

mcoker merged commit b2e003d into patternfly:master Jun 21, 2019

patternfly-build added the released label Jun 21, 2019



		// typography
		$pf-chart-global--FontSize: $pf-global--FontSize--sm;

	$pf-chart-tooltip--pointerEvents: "none";
	$pf-chart-tooltip--PointerEvents: "none";

	$pf-chart-tooltip--labels--pointerEvents: "none"; // grayscale theme
	$pf-chart-tooltip--labels--PointerEvents: "none"; // grayscale theme

	$pf-chart-voronoi--labels--pointerEvents: "none";
	$pf-chart-voronoi--labels--PointerEvents: "none";

	$pf-chart-voronoi--flyout--pointerEvents: "none";
	$pf-chart-voronoi--flyout--PointerEvents: "none";

	$pf-chart-axis--grid--pointerEvents: "painted";
	$pf-chart-axis--grid--PointerEvents: "painted";

	--pf-chart-line--data--stroke--Color: #{$pf-chart-global--Fill--Color--900};
	--pf-chart-line--data--stroke--Color: #{$pf-chart-line--data--stroke--Color};

feat(charts): add remaining variables #1863

feat(charts): add remaining variables #1863

Conversation

christiemolloy commented May 28, 2019 • edited Loading

patternfly-build commented May 28, 2019 • edited Loading

gdoyle1 commented May 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christiemolloy May 30, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christiemolloy May 30, 2019 • edited Loading

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

christiemolloy May 30, 2019 • edited Loading

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

gdoyle1 May 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlabrecq left a comment • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christiemolloy commented May 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdoyle1 commented May 31, 2019 • edited Loading

mcoker commented Jun 17, 2019

mcoker Jun 17, 2019 • edited Loading

christiemolloy commented May 28, 2019 •

edited

Loading

patternfly-build commented May 28, 2019 •

edited

Loading

christiemolloy May 30, 2019 •

edited

Loading

christiemolloy May 30, 2019 •

edited

Loading

christiemolloy May 30, 2019 •

edited

Loading

gdoyle1 May 28, 2019 •

edited

Loading

dlabrecq left a comment •

edited

Loading

gdoyle1 commented May 31, 2019 •

edited

Loading

mcoker Jun 17, 2019 •

edited

Loading