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

[WIP][TSVB] Fix field value when switching aggregation type #101160

Closed
wants to merge 1 commit into from

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Jun 2, 2021

Summary

When switching between aggregations, the following UI issue sometimes occurs

Steps to reproduce:

  1. Open TSVB and configure some pipeline aggregation. I used the following one:
    image

  2. Switch Pipeline Aggreagation -> Base Aggregation e.g. Average

Actual behavior:
User see an error message on UI
image

Expected behavior:
Field should be empty

Also this PR addressed some parts of #63593:

  • JS -> TS migration was done for:
    • create_change_handler.ts
    • get_supported_fields_by_metric_type.test.ts
    • get_supported_fields_by_metric_type.ts
    • new_series_fn.ts
    • set_is_reversed.ts
  • some js files were removed:
    • get_display_name.js
    • stacked.js
    • get_value_by.js

@alexwizp alexwizp self-assigned this Jun 2, 2021
@alexwizp alexwizp added v7.14.0 v8.0.0 Feature:TSVB TSVB (Time Series Visual Builder) Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:fix labels Jun 2, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeTimeseries 505 502 -3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
visTypeTimeseries 1.7MB 1.7MB +234.0B

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

cc @alexwizp

Copy link
Contributor

@Kuznietsov Kuznietsov left a comment

Choose a reason for hiding this comment

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

LGTM. Tested locally, works as expected.

model.stacked === STACKED_OPTIONS.PERCENT &&
isPercentDisabled(seriesQuantity[model.id])
) {
if (seriesQuantity && model.stacked === STACKED_OPTIONS.PERCENT && seriesQuantity[model.id] < 2) {
Copy link
Contributor

@Kuznietsov Kuznietsov Jun 3, 2021

Choose a reason for hiding this comment

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

Nit: Don't you want to move the condition seriesQuantity[model.id] < 2 to a separate function in case, if this condition will become more complex, and describe a check by function name? It occurs two times in the code.

@@ -74,7 +73,7 @@ export const TimeseriesConfig = injectI18n(function (props) {
defaultMessage: 'Percent',
}),
value: STACKED_OPTIONS.PERCENT,
disabled: isPercentDisabled(props.seriesQuantity[model.id]),
disabled: props.seriesQuantity[model.id] < 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This is the second time this condition with magic number occures.

@alexwizp alexwizp changed the title [TSVB] Fix field value when switching aggregation type [WIP][TSVB] Fix field value when switching aggregation type Jun 3, 2021
@alexwizp alexwizp added the WIP Work in progress label Jun 3, 2021
@spalger spalger added v7.15.0 and removed v7.14.0 labels Jun 30, 2021
@mistic mistic added v7.16.0 and removed v7.15.0 labels Aug 18, 2021
@alexwizp alexwizp closed this Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:TSVB TSVB (Time Series Visual Builder) release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.1.0 WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants