-
Notifications
You must be signed in to change notification settings - Fork 525
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
Disable inline styles #1882
Disable inline styles #1882
Conversation
|
||
`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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think passing through the disableInlineStyles
prop for VictoryLabel
would be taken care of in LabelHelpers.getProps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
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.
@@ -198,7 +211,8 @@ const getBaseProps = (props, fallbackProps) => { | |||
scale, | |||
style: style.data, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a check for disableInlineStyles
with the style
prop here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -79,7 +80,8 @@ const getBaseProps = (props, fallbackProps) => { | |||
interpolation, | |||
groupComponent, | |||
style: style.data, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -99,6 +99,7 @@ Flyout.propTypes = { | |||
center: PropTypes.shape({ x: PropTypes.number, y: PropTypes.number }), | |||
cornerRadius: PropTypes.number, | |||
datum: PropTypes.object, | |||
disableInlineStyles: PropTypes.bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -99,6 +99,7 @@ Flyout.propTypes = { | |||
center: PropTypes.shape({ x: PropTypes.number, y: PropTypes.number }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, what about adding disableInlineStyles
to CommonProps.primitiveProps
instead of in every component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already have it in primitiveProps
and dataProps
, so I must have forgotten to remove it from this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
@boygirl This should be ready for re-review! I should have addressed your comments and added this prop to the type definitions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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!
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 inline
fontSize
attribute was still necessary inVictoryLabel
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