Disable inline styles #1882

becca-bailey · 2021-06-16T18:20:27Z

This is the work to add a disableInlineStyles prop to all chart components in order to work better with CSS styles and styled-components. For more information, see additional documentation in common-props.md.

There was one exception to this - an inlinefontSize attribute was still necessary in VictoryLabel in order to calculate the y position.

To test, run yarn storybook and navigate to the "Disable Inline Styles" section for each component.

Fixes #1849

boygirl · 2021-06-16T20:41:08Z

docs/src/content/common-props/common-props.md

+
+`type: boolean`
+
+The `disableInlineStyles` prop allows Victory components to work better with CSS classes or styled-components. By default, Victory provides inline styles to chart components, which will override any conflicting CSS styles. This flag will remove the inline styles, making it easier to provide custom styling for components via CSS.


In this documentation, we should draw a distinction in the difference in behavior when providing the disableInlineStyles prop to a component like VictoryBar (i.e. inline styles are removed for both data and label components) vs supplying the prop to a component like Bar

boygirl · 2021-06-16T20:48:31Z

packages/victory-pie/src/victory-pie.js

@@ -89,6 +89,7 @@ class VictoryPie extends React.Component {
    cornerRadius: PropTypes.oneOfType([CustomPropTypes.nonNegative, PropTypes.func]),
    data: PropTypes.array,
    dataComponent: PropTypes.element,
+    disableInlineStyes: PropTypes.bool,


I think for this work to be complete for VictoryPie, you'll also need to pass the disableInlineStyles prop through to Slice component via data props here: https://github.com/FormidableLabs/victory/blob/main/packages/victory-pie/src/helper-methods.js#L249

You could also save a bit of work here: https://github.com/FormidableLabs/victory/blob/main/packages/victory-pie/src/helper-methods.js#L259

I think passing through the disableInlineStyles prop for VictoryLabel would be taken care of in LabelHelpers.getProps

It turns out that the y-position of the label component is calculated using the inline font size. If there is no font size, the y-position will evaluate to NaN. For now, this fix continues to return the fontSize style attribute and use it as an inline style. If we want to calculate the size dynamically, that might be a bigger fix later.

boygirl · 2021-06-21T16:51:04Z

packages/victory-errorbar/src/helper-methods.js

@@ -198,7 +211,8 @@ const getBaseProps = (props, fallbackProps) => {
      scale,
      style: style.data,


Missing a check for disableInlineStyles with the style prop here.

I'll add it in for consistency - I'm not sure these conditionals are doing much in most cases when we already have the check in the evaluateStyle helper.

boygirl · 2021-06-21T16:51:39Z

packages/victory-line/src/helper-methods.js

@@ -79,7 +80,8 @@ const getBaseProps = (props, fallbackProps) => {
        interpolation,
        groupComponent,
        style: style.data,


boygirl · 2021-06-21T16:53:59Z

packages/victory-tooltip/src/flyout.js

@@ -99,6 +99,7 @@ Flyout.propTypes = {
  center: PropTypes.shape({ x: PropTypes.number, y: PropTypes.number }),
  cornerRadius: PropTypes.number,
  datum: PropTypes.object,
+  disableInlineStyles: PropTypes.bool,


The prop added here doesn't do anything to this primitive yet. It needs to prevent the style prop from being applied to the Path component that Flyout renders.

Oh! never mind. I remembered that logic is centralized in Helpers.evaluateStyle which actually is called earlier in this component. I got thrown by props.style in the render.

boygirl · 2021-06-21T16:59:37Z

packages/victory-tooltip/src/flyout.js

@@ -99,6 +99,7 @@ Flyout.propTypes = {
  center: PropTypes.shape({ x: PropTypes.number, y: PropTypes.number }),


Actually, what about adding disableInlineStyles to CommonProps.primitiveProps instead of in every component?

I already have it in primitiveProps and dataProps, so I must have forgotten to remove it from this file.

boygirl

@beccanelson this is looking good! I had a couple comments for consistency and organization. Can you also add the new prop to the appropriate type definitions? That can either be in this PR or a separate one.

becca-bailey · 2021-06-22T22:20:49Z

@boygirl This should be ready for re-review! I should have addressed your comments and added this prop to the type definitions.

boygirl

@beccanelson this all looks good. I'm going to merge it into the feature branch and open a PR against main to run visual regression tests before release. Thank you for this work!

Becca Bailey added 6 commits June 15, 2021 13:00

Disable inline styles for line chart

ab04ffc

Add disableInlineStyles to data props

13f090a

Add to primitive props and documentation

7de5e0e

Update victory pie and polar axis

5cf772d

Update docs and storybook

be10879

Update bar chart stories

fec2969

becca-bailey mentioned this pull request Jun 16, 2021

Disable inline styles #1880

Closed

becca-bailey marked this pull request as ready for review June 16, 2021 18:22

becca-bailey requested a review from boygirl June 16, 2021 18:22

boygirl reviewed Jun 16, 2021

View reviewed changes

Becca Bailey added 5 commits June 18, 2021 11:04

Add more stories and modify helper methods

c960b12

Add candlestick and scatter stories

b278263

Disable styles for errorbar

76f39f2

Update documentation to include two examples

0a7c381

becca-bailey linked an issue Jun 18, 2021 that may be closed by this pull request

styled-components Bar has generated classes overridden by default styles #1849

Closed

becca-bailey added the improvement label Jun 18, 2021

boygirl reviewed Jun 21, 2021

View reviewed changes

Add missing conditionals and update prop types

96638cd

becca-bailey requested a review from boygirl June 22, 2021 22:20

boygirl approved these changes Jun 23, 2021

View reviewed changes

boygirl merged commit c0d3346 into feature/disable-inline-styles Jun 23, 2021

boygirl deleted the task/disable-more-inline-styles branch June 23, 2021 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable inline styles #1882

Disable inline styles #1882

becca-bailey commented Jun 16, 2021 •

edited

Loading

boygirl Jun 16, 2021

becca-bailey Jun 18, 2021

boygirl Jun 16, 2021

boygirl Jun 16, 2021

becca-bailey Jun 18, 2021

boygirl Jun 21, 2021

becca-bailey Jun 22, 2021

boygirl Jun 21, 2021

boygirl Jun 21, 2021

boygirl Jun 21, 2021 •

edited

Loading

boygirl Jun 21, 2021

becca-bailey Jun 22, 2021

boygirl left a comment

becca-bailey commented Jun 22, 2021

boygirl left a comment


		`type: boolean`

		The `disableInlineStyles` prop allows Victory components to work better with CSS classes or styled-components. By default, Victory provides inline styles to chart components, which will override any conflicting CSS styles. This flag will remove the inline styles, making it easier to provide custom styling for components via CSS.

		@@ -198,7 +211,8 @@ const getBaseProps = (props, fallbackProps) => {
		scale,
		style: style.data,

		@@ -99,6 +99,7 @@ Flyout.propTypes = {
		center: PropTypes.shape({ x: PropTypes.number, y: PropTypes.number }),

Disable inline styles #1882

Disable inline styles #1882

Conversation

becca-bailey commented Jun 16, 2021 • 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

boygirl Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boygirl left a comment

Choose a reason for hiding this comment

becca-bailey commented Jun 22, 2021

boygirl left a comment

Choose a reason for hiding this comment

becca-bailey commented Jun 16, 2021 •

edited

Loading

boygirl Jun 21, 2021 •

edited

Loading