-
Notifications
You must be signed in to change notification settings - Fork 844
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
[DEPRECATIONS] Remove EUI Charts, Sass deprecations #2108
Conversation
Maybe we should also merge this on a fresh master (after a release) so it doesn't get mixed up with other bug fixes etc.. |
I think I'll have to keep this up to date with rebases for a week or so. We can't merge till @simianhacker is done in kibana. I just wanted to get it going to provide more imminent awareness of its arrival :) In fact, we should let @elastic/cloud-ui know and make sure they're not using the charts either.
Yep. I'll take care of it. |
Yep, we're not using any charts from EUI |
I know this is still in draft, but we'll want to make sure to run |
Yep, purposely didn't run it in the draft since I can better watch when i need to rebase (yarn.lock changes a bunch / will always conflict) I'll add some todos as a reminder though. |
Will these charts be replaced with any new ones, or are we removing the concept of EUI charts entirely? |
@cristina-eleonora The EUI charts are being deprecated in favor of using elastic-charts which we will be adding a few examples and documentation to EUI as well. |
Awesome, thanks @cchaos ! |
Thanks to @simianhacker and elastic/kibana#41262 we're ready to merge this into EUI. I updated the This is ready for review. |
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.
Looks fine to me. I can explicitly test it against Cloud if you like, though I don't believe we're using anything that the PR is removing.
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.
Local build and docs looks good.
Will have to wait to test it in Kibana, but I agree with @chandlerprall that this is a breaking change.
@chandlerprall @thompsongl. Good point. Updated. |
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.
changes LGTM
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.
Looks good!!
Summary
Following our deprecation schedule #1469 this removes the Eui Charts set of components and docs. I did the following
experimental
folders and code (since we don't use it anymore)react-vis
from our depsk6
theme and dist outputWe can't merge this till @simianhacker removes
EuiSeriesChart
usage in the infra-ui, but he's promised soonish there. Until that's ready I'll leave this in draft mode.Checklist
This was checked in mobileThis was checked in IE11Any props added have proper autodocsDocumentation examples were addedThis was checked against keyboard-only and screenreader scenariosThis required updates to Framer X components