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

Initial visualizations plugin. #35625

Merged
merged 6 commits into from
Jun 11, 2019

Conversation

lukeelmers
Copy link
Member

@lukeelmers lukeelmers commented Apr 25, 2019

Introduces the initial visualizations plugin which will serve as the home for any vis-specific infrastructure required to power visualization development. This initial PR covers items that are currently exported by ui/vis.

Next I will need to do ui/visualize and ui/vislib. To keep this PR to a manageable size, I'd like to merge what we have first, then go back to add more services in subsequent PRs.

This initial PR includes 3 main items (easiest to review commit-by-commit):

  1. Adds base plugin framework and vis types service 74c31fb
  2. Moves ui/utils/brush_event to vis_filters (I discovered that's the only place it gets consumed, so thought it best to move in this PR while creating the vis filters service) 2c649a4
  3. Adds vis filters service 97a659c

After this is merged I will create follow-up PRs updating downstream imports to pull from this plugin instead of ui/vis.

No functional changes are happening here, as nothing is even importing this code yet, so should be safe to merge assuming we are okay with the design/structure of the services.

@elasticmachine

This comment has been minimized.

@lukeelmers lukeelmers force-pushed the feat/visualizations-plugin branch from 0fbaef9 to 44ff12a Compare April 25, 2019 21:37
@elasticmachine

This comment has been minimized.

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@lukeelmers lukeelmers requested review from lizozom and mattkime April 26, 2019 21:11
@lukeelmers lukeelmers added the release_note:skip Skip the PR/issue when compiling release notes label Apr 26, 2019
@lukeelmers lukeelmers force-pushed the feat/visualizations-plugin branch from 44ff12a to 202cd95 Compare April 29, 2019 21:27
@elasticmachine

This comment has been minimized.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

LGTM

}

/** @public types */
export {
Copy link
Member

Choose a reason for hiding this comment

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

the naming got me very confused :)

we have a VisTypesService under vis_types but here we are just re-exporting types from vis_types :) and the import on top is just importing the service and setup type

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah we are exporting all of the types for all services at the top level here. So for simplicity I'm just directly exporting them. However, for the setup type, we need to compose that into the VisualizationsSetup interface, and it doesn't really need to be exported by itself.

@lukeelmers lukeelmers force-pushed the feat/visualizations-plugin branch from 83037af to 8c20d29 Compare June 7, 2019 23:20
@elasticmachine

This comment has been minimized.

@lukeelmers

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@lukeelmers

This comment has been minimized.

@lukeelmers lukeelmers force-pushed the feat/visualizations-plugin branch from 8c20d29 to e690794 Compare June 10, 2019 17:56
@lukeelmers lukeelmers force-pushed the feat/visualizations-plugin branch from e690794 to 761ca4f Compare June 10, 2019 17:57
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lukeelmers lukeelmers removed the review label Jun 10, 2019
// @ts-ignore
import { VisProvider as Vis } from 'ui/vis/index.js';
// @ts-ignore
import { VisFactoryProvider as VisFactory } from 'ui/vis/vis_factory';
Copy link
Member Author

Choose a reason for hiding this comment

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

@ppisljar Based on your comments I renamed VisProvider -> Vis and VisFactoryProvider -> VisFactory here. When I do a pass to update #35764 with the new imports, I can also rename any instances where they are used.

If you think it will be confusing to temporarily have something exposed as Vis even though it is consumed as a provider, we can revert back; I don't have strong feelings on this either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes v7.3.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants