Skip to content

fixing missing value display for number fields#16644

Merged
ppisljar merged 2 commits intoelastic:masterfrom
ppisljar:fix/metricAggSupport
May 8, 2018
Merged

fixing missing value display for number fields#16644
ppisljar merged 2 commits intoelastic:masterfrom
ppisljar:fix/metricAggSupport

Conversation

@ppisljar
Copy link
Copy Markdown
Contributor

@ppisljar ppisljar commented Feb 9, 2018 •
edited
Loading

handles missing values by showing - (using number field formatters on them caused errors)

@ppisljar ppisljar added review Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v6.2.1 labels Feb 9, 2018
@timroes timroes added v6.2.2 and removed v6.2.1 labels Feb 9, 2018
@timroes
Copy link
Copy Markdown
Contributor

timroes commented Feb 9, 2018 •
edited
Loading

I think this doesn't fix #16640, but rather #16420 which is the specific issue for the metrics visualization.

I am also wondering: do we really need to create that metric vis specific "no data solution" that has been broken "forever" in a patch version? Or could we wait for the next minor version to fix #16640 and have a proper solution for all visualizations? (At least if we merge this, we should think about removing that quick solution in favor of the general solution once #16640 is solved.)

@jimgoodwin jimgoodwin added v6.2.3 and removed v6.2.2 labels Feb 20, 2018
@timroes
Copy link
Copy Markdown
Contributor

timroes commented Mar 12, 2018

Jenkins, test this

@ppisljar ppisljar force-pushed the fix/metricAggSupport branch from 8deb3a9 to a648f18 Compare March 13, 2018 11:44
@thomasneirynck thomasneirynck mentioned this pull request Mar 19, 2018
Merged
@jimgoodwin jimgoodwin added v6.2.4 and removed v6.2.3 labels Mar 20, 2018
@ppisljar ppisljar force-pushed the fix/metricAggSupport branch from a648f18 to 358ac33 Compare April 4, 2018 14:45
@ppisljar ppisljar changed the title adding metric support for percentiles back fixing missing value display for number fields Apr 4, 2018
@ppisljar ppisljar force-pushed the fix/metricAggSupport branch 2 times, most recently from b71afab to 5c68a62 Compare April 4, 2018 16:53
@elastic elastic deleted a comment from elasticmachine Apr 4, 2018
@elastic elastic deleted a comment from elasticmachine Apr 4, 2018
@elastic elastic deleted a comment from elasticmachine Apr 4, 2018
@elastic elastic deleted a comment from elasticmachine Apr 4, 2018
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Apr 4, 2018

💚 Build Succeeded

thomasneirynck
thomasneirynck reviewed Apr 11, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

can this be closed?

this is an isolated spotfix that addresses the issue. I'd be +1 on merging this in, iso of waiting for the "right" solution. but no big preference either way.

@ppisljar
Copy link
Copy Markdown
Contributor Author

ppisljar commented Apr 12, 2018

@timroes what do you think ?

@ppisljar ppisljar removed the v6.2.4 label Apr 16, 2018
timroes
timroes reviewed May 4, 2018
View reviewed changes
src/core_plugins/metric_vis/public/metric_vis_controller.js Outdated
Copy link
Copy Markdown
Contributor

@timroes timroes May 4, 2018

Choose a reason for hiding this comment

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

We could just use if(_.isNaN(value)) here

timroes
timroes approved these changes May 4, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Besides the small suggestions, LGTM

@ppisljar ppisljar force-pushed the fix/metricAggSupport branch from 5c68a62 to 98a7b9d Compare May 7, 2018 16:51
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented May 7, 2018

💔 Build Failed

@ppisljar
Copy link
Copy Markdown
Contributor Author

ppisljar commented May 7, 2018

jenkins, test this

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented May 7, 2018

💚 Build Succeeded

@ppisljar ppisljar requested a review from nreese May 7, 2018 22:24
nreese
nreese approved these changes May 7, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

lgtm

@ppisljar ppisljar merged commit 3c898a5 into elastic:master May 8, 2018
ppisljar added a commit to ppisljar/kibana that referenced this pull request May 8, 2018
@ppisljar
fixing missing value display for number fields (elastic#16644)
8dc8884
@ppisljar ppisljar mentioned this pull request May 8, 2018
Merged
@ppisljar ppisljar deleted the fix/metricAggSupport branch May 8, 2018 11:04
ppisljar added a commit that referenced this pull request May 8, 2018
@ppisljar
fixing missing value display for number fields (#16644) (#18902)
ee36ead
@ppisljar ppisljar restored the fix/metricAggSupport branch September 26, 2018 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nreese nreese nreese approved these changes

+2 more reviewers

@thomasneirynck thomasneirynck thomasneirynck left review comments

@timroes timroes timroes approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ppisljar @timroes @elasticmachine @nreese @thomasneirynck @jimgoodwin