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

[DEPRECATIONS] Remove EUI Charts, Sass deprecations #2108

Merged
merged 9 commits into from
Jul 18, 2019

Conversation

snide
Copy link
Contributor

@snide snide commented Jul 11, 2019

Summary

Following our deprecation schedule #1469 this removes the Eui Charts set of components and docs. I did the following

  • Removed the components and docs for EUI charts
  • Removed all references to experimental folders and code (since we don't use it anymore)
  • Removed react-vis from our deps
  • Remove the k6 theme and dist output
  • In addition, I removed all Sass deprecations we had listed in the Sass codebase. Most of them were over 6 months old.

We 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 mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios
  • This required updates to Framer X components

@cchaos
Copy link
Contributor

cchaos commented Jul 12, 2019

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..

@cchaos
Copy link
Contributor

cchaos commented Jul 12, 2019

Can we also deprecate the K6 theme in the theme switcher?

@snide
Copy link
Contributor Author

snide commented Jul 12, 2019

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.

Can we also deprecate the K6 theme in the theme switcher?

Yep. I'll take care of it.

@andrew-moldovan
Copy link

Yep, we're not using any charts from EUI

@thompsongl
Copy link
Contributor

I know this is still in draft, but we'll want to make sure to run yarn and commit the resulting yarn.lock when ready. I expect we'll see a big diff there.

@snide
Copy link
Contributor Author

snide commented Jul 12, 2019

I know this is still in draft, but we'll want to make sure to run yarn and commit the resulting yarn.lock when ready. I expect we'll see a big diff there.

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.

@cristina-eleonora
Copy link

Will these charts be replaced with any new ones, or are we removing the concept of EUI charts entirely?

@cchaos
Copy link
Contributor

cchaos commented Jul 16, 2019

@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.

@cristina-eleonora
Copy link

Awesome, thanks @cchaos !

@snide snide marked this pull request as ready for review July 18, 2019 02:36
@snide
Copy link
Contributor Author

snide commented Jul 18, 2019

Thanks to @simianhacker and elastic/kibana#41262 we're ready to merge this into EUI. I updated the yarn.lock and removed all references to the k6 theme. Note that I see a couple spots in Kibana that are pulling in the k6 JSON files (looks like within beats), so we'll want to adjust those to point to the EUI one on our next Kibana update.

This is ready for review.

@snide snide added deprecations Contains deprecations. Add them to the deprecations meta ticket after merge. developer experience labels Jul 18, 2019
@snide snide mentioned this pull request Jul 18, 2019
34 tasks
@snide snide requested a review from thompsongl July 18, 2019 03:22
Copy link
Contributor

@pugnascotia pugnascotia left a 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.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@thompsongl thompsongl left a 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.

@snide
Copy link
Contributor Author

snide commented Jul 18, 2019

@chandlerprall @thompsongl. Good point. Updated.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

changes LGTM

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.

Looks good!!

@snide snide merged commit d753291 into elastic:master Jul 18, 2019
@snide snide deleted the deprecations branch July 18, 2019 17:41
@snide snide mentioned this pull request Jul 22, 2019
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change deprecations Contains deprecations. Add them to the deprecations meta ticket after merge. developer experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants