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

Migrate chrome injected vars API to new platform #22911

Merged
merged 11 commits into from
Sep 14, 2018

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Sep 10, 2018

One of the last parts of #20696, adds methods to the InjectedMetadata service for reading one or all of the injected vars and updates ui/chrome to consume them. No changes were made to the public API so everything should behave just as it always has.

@spalger spalger added blocked Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.0.0 v6.5.0 labels Sep 10, 2018
@elasticmachine

This comment has been minimized.

@spalger spalger force-pushed the migrate-new-platform/injected-vars branch from c508e54 to ce20392 Compare September 11, 2018 15:45
@spalger spalger added review and removed blocked labels Sep 11, 2018
@azasypkin
Copy link
Member

ack: I can review this PR on Monday only (I'm off for this week) unless Court will review it earlier.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

Copy link
Contributor

@epixa epixa left a comment

Choose a reason for hiding this comment

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

I did a pass on the code and had some comments. I'll check it out locally now.

src/ui/public/chrome/api/injected_vars.ts Outdated Show resolved Hide resolved
src/ui/public/chrome/api/injected_vars.ts Outdated Show resolved Hide resolved
src/core/public/ui_settings/ui_settings_service.ts Outdated Show resolved Hide resolved
@elasticmachine

This comment has been minimized.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@epixa epixa left a comment

Choose a reason for hiding this comment

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

LGTM

@spalger spalger merged commit 931022b into elastic:master Sep 14, 2018
spalger pushed a commit to spalger/kibana that referenced this pull request Sep 14, 2018
* [core/public/injectedMetadat] migrate injected metadata APIs

* [ui/replaceInjectedVars] fix test

* [tests/browser] move injectedVars out of legacyMetadata

* [core/public/injectedMetadata] add types for legacyMetadata

* [tests/browser] move uiSettings back into legacyMetadata

* [tests/browser] fix old references to legacyMetadata

* [tests/browser] remove old tests

* [core/public/deepFreeze] fix snapshot for removal of ReadonlyArray

* [chrome/apis] use slightly less permissive types for `chrome` argument
spalger pushed a commit that referenced this pull request Sep 14, 2018
* [core/public/injectedMetadat] migrate injected metadata APIs

* [ui/replaceInjectedVars] fix test

* [tests/browser] move injectedVars out of legacyMetadata

* [core/public/injectedMetadata] add types for legacyMetadata

* [tests/browser] move uiSettings back into legacyMetadata

* [tests/browser] fix old references to legacyMetadata

* [tests/browser] remove old tests

* [core/public/deepFreeze] fix snapshot for removal of ReadonlyArray

* [chrome/apis] use slightly less permissive types for `chrome` argument
@spalger
Copy link
Contributor Author

spalger commented Sep 14, 2018

6.5/6.x: 816cd17

@spalger spalger deleted the migrate-new-platform/injected-vars branch September 14, 2018 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v6.5.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants