Closes #2541: x-axis improvements. #2542
Conversation
emtwo
force-pushed
the
emtwo/xaxis
branch
2 times, most recently
from
4f9c851 to
f30f6a0
Compare
client/app/services/query-result.js
Outdated
| @@ -102,6 +102,9 @@ function QueryResultService($resource, $timeout, $q) { | |||
| } else if (isString(v) && v.match(/^\d{4}-\d{2}-\d{2}$/)) { | |||
| row[k] = moment.utc(v); | |||
| newType = 'date'; | |||
| } else if (v.match(/^\d{4}\d{2}\d{2}$/)) { | |||
There was a problem hiding this comment.
I think, this part of code (detecting date/time values) should be extended even more: it should accept all string in format YYYY<delimiter>MM<delimiter>DD (with optional T and the end) where <delimiter> is one of :/-. or empty. @arikfr WDYT?
There was a problem hiding this comment.
Perhaps this? ^\d{4}[:\/\-.]*\d{2}[:\/\-.]*\d{2}T*$
There was a problem hiding this comment.
moment can handle this properly, but this regex also matches strings like 2018-05.16, and I'm not sure if it's okay.
There was a problem hiding this comment.
hm. might be a more complicated regex statement to make sure both delimiters match... perhaps we can leave this as-is for now and have that in a separate issue?
| scope.xAxisScales = [ | ||
| { label: 'default', value: '-' }, | ||
| { label: 'datetime' }, | ||
| { label: 'linear' }, |
There was a problem hiding this comment.
I think it's better to add value field to all items and remove that || in template.
client/app/services/query-result.js
Outdated
| @@ -102,6 +102,9 @@ function QueryResultService($resource, $timeout, $q) { | |||
| } else if (isString(v) && v.match(/^\d{4}-\d{2}-\d{2}$/)) { | |||
| row[k] = moment.utc(v); | |||
| newType = 'date'; | |||
| } else if (v.match(/^\d{4}\d{2}\d{2}$/)) { | |||
There was a problem hiding this comment.
Unless I'm missing something, this regex has potential for a lot of false detections. Basically any numeric column in the range of 10000000-99999999 will be detected as dates, no?
There was a problem hiding this comment.
Yes, you're right. I took the regex used above and removed the dashes but that makes the range of error worse than above. I've updated with a new regex pattern that is a little more restrictive.
| <ui-select-match placeholder="Choose Scale...">{{$select.selected | capitalize}}</ui-select-match> | ||
| <ui-select-choices repeat="scaleType in xAxisScales"> | ||
| <div ng-bind-html="scaleType | capitalize | highlight: $select.search"></div> | ||
| <ui-select-match placeholder="Choose Scale...">{{$select.selected.label | capitalize}}</ui-select-match> |
There was a problem hiding this comment.
The label field is already supposed to be human friendly string, why not properly captialize instead of relaying on this filter?
| @@ -77,7 +77,13 @@ function ChartEditor(ColorPalette, clientConfig) { | |||
| scope.chartTypes.custom = { name: 'Custom', icon: 'code' }; | |||
| } | |||
|
|
|||
| scope.xAxisScales = ['datetime', 'linear', 'logarithmic', 'category']; | |||
| scope.xAxisScales = [ | |||
| { label: 'default', value: '-' }, | |||
There was a problem hiding this comment.
Maybe Auto Detect is a better description?
|
@arikfr I addressed your comments. In terms of the regex concern, please see the comment I left. I updated it to be more restrictive but if we're still not ok with this, perhaps we can remove it entirely from this PR. For some context, the motivation for that change originally came from this issue in the mozilla fork: mozilla#338. |
|
@emtwo thank you for the updates! As for the regex, while it limits the range, it can still wrongly detect large numbers as dates. As this is a Mozilla specific issue, I would prefer that you address it in some other way. Some ideas --
In the longer term, it can be great if the user could define the field types of the result set. Similar to how we do it now for the table visualization, but in a global way that will be used in all the visualizations. But this will take some time to implement properly... |
|
@arikfr Thank you for those ideas! Indeed this is a fairly Mozilla-specific issue so I have taken that change out of this PR and we can re-evaluate how we'll handle it on our side. This PR is now only for adding |
|
Thanks! |
No description provided.