-
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] Update style when metrics change #83586
[Maps] Update style when metrics change #83586
Conversation
Pinging @elastic/kibana-gis (Team:Geo) |
@elasticmachine merge upstream |
x-pack/plugins/maps/public/classes/styles/vector/properties/dynamic_style_property.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/maps/public/classes/styles/vector/style_fields_helper.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/maps/public/classes/styles/vector/vector_style.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/maps/public/classes/styles/vector/vector_style.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/maps/public/classes/styles/vector/vector_style.tsx
Outdated
Show resolved
Hide resolved
@@ -18,6 +18,7 @@ import { | |||
VECTOR_STYLES, | |||
RawValue, | |||
FieldFormatter, | |||
TOP_TERM_PERCENTAGE_SUFFIX, |
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.
Clean up all import changes in this file, since they are no longer used
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.
Looks like you missed cleaning up these imports
@@ -290,7 +316,6 @@ export function updateSourceProp( | |||
if (newLayerType) { | |||
dispatch(updateLayerType(layerId, newLayerType)); | |||
} | |||
await dispatch(clearMissingStyleProperties(layerId)); |
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.
only needs to occur for metrics changes
) { | ||
const styleFieldsHelper = await createStyleFieldsHelper(nextFields); | ||
|
||
return previousFields.length === nextFields.length |
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.
this assumption only works when field-descriptors are complete. #84828 will address the issue of storing invalid field-descriptors in the store.
@@ -18,6 +18,7 @@ import { | |||
VECTOR_STYLES, | |||
RawValue, | |||
FieldFormatter, | |||
TOP_TERM_PERCENTAGE_SUFFIX, |
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.
Looks like you missed cleaning up these imports
x-pack/plugins/maps/public/classes/styles/vector/vector_style.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/maps/public/classes/styles/vector/vector_style.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/maps/public/classes/styles/vector/vector_style.tsx
Outdated
Show resolved
Hide resolved
styleFieldsHelper: StyleFieldsHelper, | ||
originalProperties: VectorStylePropertiesDescriptor, | ||
mapColors: string[], | ||
hasChanges: boolean |
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.
No need to pass this in since _updateFieldsInDescriptor no longer calls this function.
x-pack/plugins/maps/public/classes/styles/vector/vector_style.tsx
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.
Just to small nits about not exporting location functions. This will making editing layers with metrics a much better experience. Its great that styles using metrics will auto-update to the new metric when possible and save users a lot of clicks.
LGTM
code review, tested in chrome
Co-authored-by: Kibana Machine <[email protected]>
💔 Build Failed
Failed CI Steps
Test FailuresFirefox UI Functional Tests.test/functional/apps/dashboard/index·js.dashboard app using current data "before all" hook: loadCurrentData in "using current data"Standard Out
Stack Trace
Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/monitoring/elasticsearch/nodes·js.Monitoring app Elasticsearch nodes listing with offline node should sort by statusStandard Out
Stack Trace
Jest Integration Tests.src/core/server/ui_settings/integration_tests.uiSettings/routes doc missing get route creates doc, returns a 200 with settingsStandard Out
Stack Trace
and 20 more failures, only showing the first 3. Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
Summary
This PR and #84828 will close #82892.
When a user changes a metric, the style-properties get "unset", e.g. causing icons to shrink in size or colors to go black.
With this PR, style properties will automatically update their field-configuration, based on changes in the metrics-configuration.
This is blocking #83393
#84828 addresses an edge-case. Metric-descriptors can go into an invalid state (e.g. due to missing field-name selection) but are still stored in the redux-store. This causes invalid metrics to be dropped when they are serialized in IField-objects.
Checklist
Delete any items that are not applicable to this PR.
For maintainers