-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
fixing missing value display for number fields #16644
Conversation
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.) |
Jenkins, test this |
8deb3a9
to
a648f18
Compare
a648f18
to
358ac33
Compare
b71afab
to
5c68a62
Compare
💚 Build Succeeded |
There was a problem hiding this 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.
@timroes what do you think ? |
@@ -65,6 +65,11 @@ export class MetricVisComponent extends Component { | |||
return isColorDark(parseInt(color[1]), parseInt(color[2]), parseInt(color[3])); | |||
} | |||
|
|||
_getFormattedValue(fieldFormatter, value) { | |||
if (!value && isNaN(value)) return '-'; |
There was a problem hiding this comment.
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
There was a problem hiding this 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
5c68a62
to
98a7b9d
Compare
💔 Build Failed |
jenkins, test this |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
handles missing values by showing - (using number field formatters on them caused errors)