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.

Sign up for GitHub

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

Jump to bottom

[Visualize] Bar chart: Show missing values on chart setting #66375

Merged
merged 7 commits into from
May 19, 2020
Merged

[Visualize] Bar chart: Show missing values on chart setting #66375

merged 7 commits into from
May 19, 2020

Conversation

sulemanof
Copy link
Contributor

Summary

Fixes #66365

Rely on vis.type.name instead of vis.type.type, since the last one is gone.
Can't rely on vis.params.type, since it is not changed when a chart type is changed via select in Metrics & axes panel:

image

Checklist

Delete any items that are not applicable to this PR.

For maintainers

ppisljar
ppisljar approved these changes May 13, 2020
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM, could we add a test for this ?

@spalger spalger added v7.7.1 and removed v7.7.0 labels May 14, 2020
@timroes
Copy link
Contributor

timroes commented May 14, 2020

I think we should somehow add a functional test for that. Though this would need to check the chart, which will as soon as we switch to elastic-charts (and thus canvas rendering) not be as easy anymore. But we should at least check it for now, and maybe later need to convert it to screenshot tests.

@timroes
Copy link
Contributor

timroes commented May 19, 2020

@elasticmachine merge upstream

flash1293
flash1293 reviewed May 19, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested in Firefox and works as expected. I noticed one strange behavior, not sure whether introduced by this PR:

  • Create a bar chart with two metrics - "show values" is available
  • Change the first metric to line - "show values" is available (which makes sense as there is still a "bar" metric)
  • Change the second metric to area - "show values" is available (doesn't make sense, as it's not applied anymore)
  • Change the second metric to line as well - "show values" is not available any more

Screenshot 2020-05-19 at 10 51 00

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@sulemanof
Copy link
Contributor Author

sulemanof commented May 19, 2020 •
edited
Loading

Thanks @flash1293 for the good points you mentioned!
This was not introduced by this PR, but lives for a long.
If this PR is a good place to fix it, I could implement something like:
https://github.com/elastic/kibana/blob/master/src/plugins/vis_type_vislib/public/components/options/metrics_axes/index.tsx#L296
and also update FT to check different chart types and their mix.

Updated:
Checked in 7.3.1, where the feature was introduced #36511,
and there is the same bug exist: (line & area metrics)

image

@flash1293
Copy link
Contributor

@sulemanof In that case this LGTM

@sulemanof sulemanof marked this pull request as ready for review May 19, 2020 11:27
@sulemanof sulemanof requested a review from a team May 19, 2020 11:27
@sulemanof sulemanof added :KibanaApp/fix-it-week release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure labels May 19, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

flash1293
flash1293 approved these changes May 19, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM for GH

@sulemanof sulemanof merged commit b70955b into elastic:master May 19, 2020
@sulemanof sulemanof deleted the fix/bar_chart_values branch May 19, 2020 14:47
@sulemanof sulemanof mentioned this pull request May 19, 2020
Merged
sulemanof added a commit to sulemanof/kibana that referenced this pull request May 19, 2020
@sulemanof @elasticmachine
[Visualize] Bar chart: Show missing values on chart setting (elastic#…
6a470d9 
…66375)

* Show missing switch button

* Add functional tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@sulemanof sulemanof mentioned this pull request May 19, 2020
Merged
sulemanof added a commit to sulemanof/kibana that referenced this pull request May 19, 2020
@sulemanof
[Visualize] Bar chart: Show missing values on chart setting (elastic#…
ff1f449 
…66375)

* Show missing switch button

* Add functional tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	src/plugins/vis_type_vislib/public/components/options/point_series/point_series.tsx
#	test/functional/page_objects/visualize_chart_page.ts
#	test/functional/page_objects/visualize_editor_page.ts
@sulemanof sulemanof mentioned this pull request May 19, 2020
Merged
sulemanof added a commit that referenced this pull request May 19, 2020
@sulemanof @elasticmachine
[Visualize] Bar chart: Show missing values on chart setting (#66375) (#…
a474b78 
…67017)

* Show missing switch button

* Add functional tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
sulemanof added a commit that referenced this pull request May 19, 2020
@sulemanof @elasticmachine
[Visualize] Bar chart: Show missing values on chart setting (#66375) (#…
12a8769 
…67019)

* Show missing switch button

* Add functional tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
sulemanof added a commit that referenced this pull request May 19, 2020
@sulemanof
[Visualize] Bar chart: Show missing values on chart setting (#66375) (#…
7a7c9f9 
…67025)

* Show missing switch button

* Add functional tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	src/plugins/vis_type_vislib/public/components/options/point_series/point_series.tsx
#	test/functional/page_objects/visualize_chart_page.ts
#	test/functional/page_objects/visualize_editor_page.ts
gmmorris added a commit to gmmorris/kibana that referenced this pull request May 20, 2020
@gmmorris
Merge branch 'master' into saved-objects/scoped-client-with-extra-types
244da07 
* master: (33 commits)
  [Saved Objects] adds support for including hidden types in saved objects client (elastic#66879)
  [Discover] Deangularize timechart header (elastic#66532)
  [Discover] Improve and unskip a11y context view test (elastic#66959)
  [SIEM] Refactor Timeline.timelineType draft to Timeline.status draft (elastic#66864)
  docs: update RUM documentation link (elastic#67042)
  [QA] fixup coverage ingestion tests. (elastic#66905)
  [Metrics UI] Add support for multiple groupings to Metrics Explorer (and Alerts) (elastic#66503)
  [Metrics UI] Add sorting for name and value to Inventory View (elastic#66644)
  [Metrics UI] Change Metric Threshold Alert charts to use bar charts (elastic#66672)
  [Uptime] Use React.lazy for alert type registration (elastic#66829)
  [Reporting] Consolidate API Integration Test configs (elastic#66637)
  Allow histogram fields in average and sum aggregations (elastic#66891)
  Fix saved object share link (elastic#66771)
  move role reset into the top level after clause (elastic#66971)
  Automate the labels for any PRs affecting files for the Ingest Management team (elastic#67022)
  [SIEMDPOINT] Move endpoint to siem (elastic#66907)
  server.uuid so is not used (elastic#66963)
  Revert "[ci/stats] fix git metadata collection (elastic#66840)"
  [Uptime] Unmount uptime app properly (elastic#66950)
  [Visualize] Bar chart: Show missing values on chart setting (elastic#66375)
  ...
@sulemanof sulemanof mentioned this pull request Oct 1, 2020
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppisljar ppisljar ppisljar approved these changes

@flash1293 flash1293 flash1293 approved these changes

@timroes timroes Awaiting requested review from timroes

Assignees
No one assigned
Labels
backported release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.7.1 v7.8.0 v7.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Show values on chart setting is gone
7 participants
@sulemanof @timroes @kibanamachine @flash1293 @elasticmachine @ppisljar @spalger