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

Fix react-vis overrides #1123

Merged
merged 4 commits into from
Aug 20, 2018
Merged

Fix react-vis overrides #1123

merged 4 commits into from
Aug 20, 2018

Conversation

markov00
Copy link
Member

Fix #1121
On the current implementation we copied the existing react-vis style into EUI.
We also overridden few classes.
Since react-vis is also used on APM, I've scoped all the react-vis class behind a container class.
BTW this is a workaround until the new version of the EUIChart is ready.

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! 🙏

@jasonrhodes
Copy link
Member

FWIW @markov00, in the <XYPlot> component you should be able to pass it a className prop which will append your custom class alongside .rv-xy-plot, if you like.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 @markov00 Thanks for jumping on this fix!

@@ -128,7 +128,10 @@ class XYChart extends PureComponent {
const Crosshair = orientation === HORIZONTAL ? EuiCrosshairY : EuiCrosshairX;
const seriesNames = this._getSeriesNames(children);
return (
<div {...rest}>
<div
className="euiSeriesChartContainer"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a fine way to do it (albeit, for now) since this container already exists and you don't need to then create classnames for each individual plot.

@cchaos
Copy link
Contributor

cchaos commented Aug 17, 2018

Oh and don't forget a changelog entry and double check your tests, I see the latest build errored.

@markov00
Copy link
Member Author

@cchaos sure, It's a bit strange why the test fails, when I ran the tests I've also got some other snapshot updated like switch.test.js.snap icon.test.js.snap and I've pushed also them. But on CI it reject this changes. Maybe a different version of some node modules. I've to check and push it again.

@cchaos I can also change this to enable composition of external classnames together with the one on eui

@jasonrhodes yes sure, the big problem with react-vis is that not every component has the possibility to pass a class, some of them have already some style applied that override your class so we preferred to import their scss files and change them directly.

@markov00 markov00 merged commit 91dbf4e into elastic:master Aug 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants