Skip to content

Date Range Agg - key needs to be a string#58938

Closed
majagrubic wants to merge 3 commits intoelastic:masterfrom
majagrubic:fix-57122
Closed

Date Range Agg - key needs to be a string#58938
majagrubic wants to merge 3 commits intoelastic:masterfrom
majagrubic:fix-57122

Conversation

@majagrubic
Copy link
Contributor

Summary

Attempt at fixing: #57122.

The problem here is that d3 cannot take domain in the form {from, to}:
https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/vis_type_vislib/public/vislib/lib/axis/axis_scale.js#L246
It expect a value or an array. (Credits to Marco for figuring this out). This breaks the scale calculation and was probably introduced in this PR:
#48090

I think the reasonable solution would be to return to the previous state, ie. returning string key instead of an object. I'm not sure what are the implications of doing this and if something else would break. I tested this on the vertical bar with Date Range on X-Axis.

Before:
Screenshot 2020-02-26 at 13 52 46

After:
Screenshot 2020-02-28 at 20 03 20

Let me know what you think and if it's ok to continue down this path.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@majagrubic majagrubic requested a review from flash1293 February 28, 2020 20:13
@majagrubic majagrubic changed the title Date Range vis - range needs to be a key Date Range Vis - key needs to be a string Feb 28, 2020
@majagrubic majagrubic changed the title Date Range Vis - key needs to be a string Date Range Agg - key needs to be a string Feb 28, 2020
@flash1293
Copy link
Contributor

You are completely right @majagrubic , that's the PR which caused the bug. I would recommend not to revert the change though because we need access to the raw object of the range to be able to correctly build the filter when clicking a bar / pie slice in a chart in src/legacy/core_plugins/vis_type_vislib/public/vislib/lib/dispatch.js

If you want to continue to look into this (happy to take it over if you want), I would recommend trying to stringify the value somehow to make it compatible with d3, e.g. before passing it into https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/vis_type_vislib/public/vislib/lib/axis/axis_scale.js#L246

@kibanamachine
Copy link
Contributor

💔 Build Failed

History

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

@majagrubic majagrubic closed this Apr 8, 2020
@majagrubic majagrubic deleted the fix-57122 branch April 8, 2020 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants