Skip to content

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

Copy link
Contributor

@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

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.

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

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 Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// labels May 19, 2020
@elasticmachine
Copy link
Contributor

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

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 added a commit to sulemanof/kibana that referenced this pull request May 19, 2020
…66375)

* Show missing switch button

* Add functional tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
sulemanof added a commit to sulemanof/kibana that referenced this pull request May 19, 2020
…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 added a commit that referenced this pull request May 19, 2020
…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
…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
…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
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backported release_note:fix Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.7.1 v7.8.0 v7.9.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Show values on chart setting is gone

7 participants