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

[Lens] fix empty state for pie #66206

Merged
merged 3 commits into from
May 13, 2020

Conversation

mbondyra
Copy link
Contributor

@mbondyra mbondyra commented May 12, 2020

Summary

Fixes #65692

The PR

  • fixes condition for empty state,
  • fixes console.error about missing IntlProvider and displaying the default message,
  • adds visual changes. I am using the icon visPie to display the icon of pie chart no matter if it's pie, donut or treemap as this is the only one we have in EUI. @cchaos do you think it's worth to design the icons for donut and treemap?

image

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@mbondyra mbondyra added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.8.0 v7.9.0 labels May 12, 2020
@mbondyra mbondyra requested review from cchaos, wylieconlon, flash1293 and a team May 12, 2020 12:07
@elasticmachine
Copy link
Contributor

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

@cchaos
Copy link
Contributor

cchaos commented May 12, 2020

do you think it's worth to design the icons for donut and treemap?

We already have custom chart type icons for Lens in the chart type switcher. I'd rather not add more glyphs as well for every chart type. I would have thought we'd just use a simple chart icon for every type of chart for all empty states. I'll consider what this should be.

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 in editor and on dashboard, works fine for me. Code LGTM, we could add a unit test for this behavior but no blocker for merging.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

I just realized you're trying to get this into 7.8. I think it's fine for now to ship with the pie chart icon for donut and treemaps. I'll continue to think on this state for 7.9.

The screenshots LGTM, though I didn't test locally. Thanks for getting it in so quickly!

@mbondyra
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@mbondyra mbondyra merged commit 0d571ae into elastic:master May 13, 2020
@mbondyra mbondyra deleted the lens/fix-empty-state-pie-chart branch May 13, 2020 12:50
mbondyra added a commit to mbondyra/kibana that referenced this pull request May 13, 2020
mbondyra added a commit to mbondyra/kibana that referenced this pull request May 13, 2020
v1v added a commit to v1v/kibana that referenced this pull request May 13, 2020
* upstream/master: (223 commits)
  [Ingest] Support root level yaml variables in agent stream template (elastic#66120)
  [Snapshot Restore] Fix error when deleting snapshots behind reverse proxy (elastic#66147)
  [Lens] fix empty state for pie (elastic#66206)
  [APM] Improve e2e tests (elastic#66373)
  [ML] Data Frame Analytics: Fix steps to be named phases. (elastic#65855)
  [Discover] Encode context link filter part (elastic#63831)
  [APM] Scope APM alert creation to environment (elastic#65681)
  Move Kibana Usage collectors outside the telemetry plugin (elastic#65663)
  [ML] Data Frame Analytics: Fix confusion matrix data grid width. (elastic#65818)
  Switch to core application service (elastic#63443)
  Removes use of prefer_v2_templates (elastic#66316)
  [Maps] handle case where fit to bounds does not match any documents (elastic#66307)
  log error instead of throw (elastic#66254)
  [plugin-helpers] remove outdated postinstall task (elastic#66324)
  Spaces - migrate default space and enter space view to KP (elastic#66098)
  [APM] Change plugin id for `apm_oss` to `apmOss` (elastic#66164)
  [Maps] convert map_selectors to TS (elastic#65905)
  [uptime/usage-collector] add missing await (elastic#66079)
  [Ingest] Add additional attributes to the Datasources Saved Object (elastic#66127)
  [Endpoint]EMT-339: add new policy response schema (elastic#66264)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.8.0 v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Pie charts should show "no data" when all data is 0s
5 participants