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

[Gauge] Vis Type #126048

Merged
merged 36 commits into from
Mar 4, 2022
Merged

Conversation

Kuznietsov
Copy link
Contributor

@Kuznietsov Kuznietsov commented Feb 21, 2022

Summary

Completes part of the issue

  • Add vis_type for gauge/goal
  • Add support of percentage mode from vis_dimension
  • Disable form options that are not working for elastic-charts

Disabled options for elastic charts

elastic-charts.options.mov

Goal

Screenshot 2022-02-22 at 17 49 47

Gauge

Screenshot 2022-02-22 at 17 50 16

Split chart warnings

Screenshot 2022-03-03 at 13 25 07

Screenshot 2022-03-03 at 13 25 22

@Kuznietsov Kuznietsov added the WIP Work in progress label Feb 21, 2022
@Kuznietsov Kuznietsov self-assigned this Feb 21, 2022
@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@Kuznietsov Kuznietsov requested a review from stratoula February 28, 2022 09:26
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Thanx Yaroslav for working on it! Some comments from my side:

  1. In vislib the users can ovewrite the gauge colors from the legend. These colors are saved with the SO (uiState). Although we dont support the legend yet, I think that we should respect the overwritten colors. So for example this is a gauge with overwritten color (orange) but when I open it on the new implementation, the color is not respected

image

image

  1. When I have a saved gauge with small multiples, and then render it with the new implementation, the user sees only one chart (makes sense as we dont support it). Can we add a warning in that case? We are doing the same for heatmap so it will be easy to integrate it.
  2. The percentage mode returns different results for the new implementation.

image

image

'visTypeGauge.controls.gaugeOptions.showOutlineNotAvailable',
{
defaultMessage:
'The outile is not supported with the new charts library. Please, enable the gauge legacy charts library advanced setting.',
Copy link
Contributor

Choose a reason for hiding this comment

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

The outline (fix typo)

title: 'Heatmap',
description: 'Creates a heatmap viz',
icon: 'visHeatmap',
name: 'gauge',
Copy link
Contributor

Choose a reason for hiding this comment

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

This mock is for heatmap, why have we altered this?

@Kuznietsov Kuznietsov added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:feature Makes this part of the condensed release notes labels Feb 28, 2022
@Kuznietsov Kuznietsov changed the title [Gauge] Vis Type [WIP][Gauge] Vis Type Feb 28, 2022
@Kuznietsov Kuznietsov added the WIP Work in progress label Feb 28, 2022
@Kuznietsov
Copy link
Contributor Author

Kuznietsov commented Mar 2, 2022

We've discussed with @stratoula in the private chat, that I'm going to implement legacy percentage mode and uiState coloring in the follow-up PRs. And Warning about small multiples at the current.

Percentage mode is going to be ready very soon: #126318.

@stratoula
Copy link
Contributor

@Kunzetsov the plan is to create additional PRs for the colouring and percentage mode and merge them to this PR or are we going to merge this PR in main and then the additional PRs?

@Kuznietsov
Copy link
Contributor Author

Kuznietsov commented Mar 3, 2022

@Kunzetsov the plan is to create additional PRs for the colouring and percentage mode and merge them to this PR or are we going to merge this PR in main and then the additional PRs?

@stratoula, To the main one by one.

@stratoula
Copy link
Contributor

@Kunzetsov I see that you have hidden the Add bucket btn
image

This is correct of course as we don't support it but I am not very fond of the fact that we have a buckets title without any action.

I can think 3 things to improve it:

  • Not display the Buckets section (not so sure if it is easily done)
  • Do it like heatmap, (display add but split chart disabled with a tooltip

image

- Display add but disabled with a tooltip

It might not be so important as the new implementation won't be the default one but I would like yours and Joe's thoughts on that cc @flash1293 .

@Kuznietsov
Copy link
Contributor Author

@stratoula, totally makes sense. I'll implement it right now. I didn't know, that we have heatmap when I was implementing this approach )

@flash1293
Copy link
Contributor

Like that approach @stratoula

@Kuznietsov Kuznietsov changed the title [WIP][Gauge] Vis Type [Gauge] Vis Type Mar 3, 2022
@Kuznietsov Kuznietsov removed the WIP Work in progress label Mar 3, 2022
@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeGauge - 20 +20
visTypeVislib 184 176 -8
visualizations 214 216 +2
total +14

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
expressionGauge 68 69 +1
visTypeGauge - 7 +7
visualizations 326 328 +2
total +10

Async chunks

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

id before after diff
expressionGauge 8.9KB 9.1KB +117.0B
visTypeGauge - 6.1KB +6.1KB
visTypeVislib 356.1KB 351.8KB -4.3KB
visualizations 164.1KB 165.1KB +976.0B
total +2.8KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
expressionGauge 3 2 -1
visTypeGauge - 2 +2
total +1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
visTypeGauge - 9.7KB +9.7KB
visTypeVislib 19.5KB 15.9KB -3.6KB
visualizations 44.8KB 44.9KB +122.0B
total +6.2KB
Unknown metric groups

API count

id before after diff
expressionGauge 68 69 +1
visTypeGauge - 7 +7
visualizations 347 349 +2
total +10

async chunk count

id before after diff
visTypeGauge - 1 +1
visTypeVislib 3 2 -1
total -0

ESLint disabled line counts

id before after diff
visTypeGauge - 3 +3
visTypeVislib 3 2 -1
total +2

Total ESLint disabled count

id before after diff
visTypeGauge - 3 +3
visTypeVislib 5 4 -1
total +2

History

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

cc @Kunzetsov

@stratoula
Copy link
Contributor

stratoula commented Mar 4, 2022

This looks great Yaroslav. I only have one comment. When the user switches off the labels, I think we should also hide the Count label.

image

Update: The vislib also keeps it (it is less prominent than the new implementation but we should keep the vislib way so ignore it :))

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

I approve it as it does what it says. The percentage mode and the uiState doesn't work but we have follow up PRs for them. With that being said, I am approving it (not a fan of merging PRs with bugs though :D )

@Kuznietsov Kuznietsov merged commit 2b6885a into elastic:main Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:medium Medium Level of Effort NeededFor:VisEditors release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants