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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Feb 27, 2020
Merged

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

merged 3 commits into from
Feb 27, 2020

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Feb 25, 2020 •
edited
Loading

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

@elastic elastic deleted a comment from kibanamachine Feb 26, 2020
ppisljar
ppisljar approved these changes Feb 26, 2020
View reviewed changes
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

@alexwizp alexwizp requested a review from Dosant February 27, 2020 08:41
@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
Dosant
Dosant approved these changes Feb 27, 2020
View reviewed changes
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

src/plugins/visualizations/public/persisted_state/persisted_state.ts
* 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

ppisljar
ppisljar approved these changes Feb 27, 2020
View reviewed changes
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
flash1293
flash1293 approved these changes Feb 27, 2020
View reviewed changes
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

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

igoristic
igoristic approved these changes Feb 27, 2020
View reviewed changes
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 @elasticmachine
[Step 2] ui/persisted_state 👉 src/plugins/visualizations (elastic#58501)
33d2ad2 
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@alexwizp alexwizp mentioned this pull request Feb 27, 2020
Merged
alexwizp added a commit that referenced this pull request Feb 28, 2020
@alexwizp @elasticmachine
[Step 2] ui/persisted_state 👉 src/plugins/visualizations (#58501) (#5…
80349f2 
…8770)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@nickofthyme nickofthyme mentioned this pull request Mar 9, 2020
Closed
@alexwizp alexwizp mentioned this pull request Mar 10, 2020
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisronline chrisronline chrisronline left review comments

@Dosant Dosant Dosant approved these changes

@flash1293 flash1293 flash1293 approved these changes

@ppisljar ppisljar ppisljar approved these changes

@igoristic igoristic igoristic approved these changes

Assignees

@alexwizp alexwizp

Labels
Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@alexwizp @ppisljar @elasticmachine @kibanamachine @chrisronline @flash1293 @Dosant @igoristic