-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Maps] Separate layer wizards for Clusters and heatmap #60870
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested locally on Chromium and Firefox, all layers work as expected.
A very much needed change in the UI; this is a great!! 🌈
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Splitting out heatmap is perfect. It will surface this functionality so much easier.
Most of the feedback is around scope. Ping to discuss.
x-pack/legacy/plugins/maps/public/layers/sources/es_geo_grid_source/es_geo_grid_source.js
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/maps/public/layers/layer_wizard_registry.ts
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,35 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is forward-thinking reasoning behind introducing the source-registry, that this could serve as an extension for 3rd party plugins to register custom sources.
But this isn't strictly necessary to achieve what we need in the short term:
- in this PR, to enable the split between clusters/heatmaps
- going forward to the more generic "curated layers" concept
I'm thinking we could drop this introduction from this PR as it just introduces additional abstractions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
source_registry is needed because layer_wizard_registry does not correlate one-to-one with sources. For example, there are 2 wizards that return geo_grid source.
all_sources is clunky and will not support or future needs.
@@ -48,16 +48,6 @@ function createLayerInstance(layerDescriptor, inspectorAdapters) { | |||
} | |||
} | |||
|
|||
function createSourceInstance(sourceDescriptor, inspectorAdapters) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see other comment. This source-registry isn't strictly necessary. We'd just have a very long switch-case statement here to instantiate the right source.
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline:
- keep source-registry. It's an extention-path we will want to introduce later anyway.
- remove ordering-param and replace it with module that registers source in desired order.
- this does not distribute ordering-info across multiple modules.
- we're not 100% sure yet how to evolve ordering-mechanism anyway, especially in context of upcoming hierarchical layer-wizards. Also, 3rd party extension likely will not be able to interject their own source into the default offerings anyway, and will need to use a different mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really neat. This cluster/heatmap split is better than what there is now, and it will only get better as this concept of curated wizards evolves.
Most of my comments are suggestions to reduce the footprint of new boilerplate when it's not strictly necessary.
x-pack/legacy/plugins/maps/public/layers/sources/source_registry.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/maps/public/layers/sources/source_registry.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/maps/public/layers/sources/source_registry.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/maps/public/layers/layer_wizard_registry.ts
Outdated
Show resolved
Hide resolved
...egacy/plugins/maps/public/connected_components/layer_addpanel/source_select/source_select.js
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/maps/public/connected_components/layer_addpanel/source_editor/view.js
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/maps/public/layers/layer_wizard_registry.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎩
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* [Maps] source registry and register seperate clusters and heat map sources * split into to registries * add EMS file source * add geojson upload layer * register rest of sources * i18n changes * ts lint errors * fix jest test * fix pew-pew source * review feedback * import registires in plugin so they exist in embeddable * remove order parameter and move all layer registies into single file * fix functionalt est * pass constructor to sourceREgistry instead of factory * review feedback Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* master: [HomeApp] Set breadcrumbs when coming back from add data dir (elastic#62186) [Lens] fix error for minInterval>computedInterval for XYChart (elastic#61931) ci: remove AppArch label from ProBot path-labeler (elastic#62211) [Uptime] Optimize get latest monitor API (elastic#61820) [Maps] Separate layer wizards for Clusters and heatmap (elastic#60870) Remove polling delay (elastic#62099) accessibility tests for dashboard panel ( OSS) (elastic#62055) rename README.md to readme, avoiding issues with case change [SIEM] [Detection Engine] Fixes all rules sorting (elastic#62039) [SIEM] CASES Bugs BC2 (elastic#62170) Revert "Endpoint: Add ts-node dev dependency (elastic#61884)" (elastic#62197) Closes elastic#60173 by turning off client caching for the main service map API call (elastic#62111) [SIEM] Restores the _External alert count_ widget's subtitle (elastic#62094) [Maps] Update ems client dependency to 7.8.0 (elastic#62181) [Metrics Alerts] Fix action variables, default message, and EU… (elastic#62061) Update CODEOWNERS with ES-UI apps, including grok debugger. (elastic#62045) Update ILM node attributes blacklist. (elastic#62093)
* [Maps] source registry and register seperate clusters and heat map sources * split into to registries * add EMS file source * add geojson upload layer * register rest of sources * i18n changes * ts lint errors * fix jest test * fix pew-pew source * review feedback * import registires in plugin so they exist in embeddable * remove order parameter and move all layer registies into single file * fix functionalt est * pass constructor to sourceREgistry instead of factory * review feedback Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
fixes #55880
This PR decouples sources from layer creation wizard by removing
ALL_SOURCES
and replacing it withsource_registry
andlayer_wizard_registry
. This will enable the following longer term goals:Add layer
so users do not see all options in ever growing unmanageable list.This PR tried to limit the scope of the changes and did not refactor things like
SourceSelect
andSourceEditor
which should be refactored to accomplish the above goals. This PR only tried to establish the basic registries.