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

[Step 2] ui/persisted_state 👉 src/plugins/visualizations #58501

Merged
merged 3 commits into from
Feb 27, 2020

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Feb 25, 2020

Part of #46926

Summary

  • ui/persisted_state 👉 src/plugins/visualizations
  • remove SimpleEventEmitter
  • remove PersistedStateError class.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

@ppisljar
Copy link
Member

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

@ppisljar
Copy link
Member

tests failed on plugin generator, this usually means that browser karma tests fail yarn test:dev to see locally

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@alexwizp alexwizp added release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration labels Feb 27, 2020
@alexwizp alexwizp self-assigned this Feb 27, 2020
@alexwizp alexwizp marked this pull request as ready for review February 27, 2020 12:10
@alexwizp alexwizp requested a review from a team February 27, 2020 12:10
@alexwizp alexwizp requested a review from a team as a code owner February 27, 2020 12:10
@alexwizp alexwizp requested a review from a team February 27, 2020 12:10
@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@ppisljar ppisljar self-requested a review February 27, 2020 13:01
Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Code lgtm

* under the License.
*/

import { EventEmitter } from 'events';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:
looks like this is not an explicit kibana dependency in package.json even-though is used in bunch of places.
I guess we should add it to package.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Dosant from my point of view too, but please see: #58606

@spalger it confuse not only me =)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for pointing me into it. I didn't know it is automatic shimming, I though it is just transitive dependency

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

@ppisljar ppisljar removed the request for review from lukeelmers February 27, 2020 13:15
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Kibana app changes LGTM, didn't test locally.

@ppisljar ppisljar removed the request for review from VladLasitsa February 27, 2020 15:51
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

  • 💔 Build #29130 failed e7fb68c6abc7896d823eee76ae83612909c9b73a
  • 💔 Build #29114 failed 4e2745bb6296afa97f5006ef88c03e9bfcfb5a72

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@igoristic igoristic 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 👍

@alexwizp alexwizp merged commit 29e984a into elastic:master Feb 27, 2020
alexwizp added a commit to alexwizp/kibana that referenced this pull request Feb 27, 2020
alexwizp added a commit that referenced this pull request Feb 28, 2020
@alexwizp alexwizp mentioned this pull request Mar 10, 2020
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants