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

Date histogram brush - add range filter when field is not index pattern time field. #12286

Merged
merged 5 commits into from
Jun 29, 2017

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jun 12, 2017

fixes #3173

Modifies how brush events on date fields are handled. When the field is the index pattern timefield, than the timefilter is updated. When the field is not the index pattern timefield, then the range filter is created.

@nreese nreese requested review from cjcenizal and ppisljar June 12, 2017 19:26
@nreese nreese added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v6.0.0 labels Jun 13, 2017
let filterType = RANGE_FILTER;
if (event.data.xAxisField.type === 'date' &&
event.data.xAxisField.name === event.data.indexPattern.timeFieldName) {
filterType = TIME_FILTER;
Copy link
Member

Choose a reason for hiding this comment

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

Is the switch really necessary? What if we just moved the block from the switch statement into the body of this if statement? Seems like it could simplify this logic a bit.

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Functionality looks good, and code changes look good! Nice work @nreese!

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

Hmm, seems like there might be a lingering issue here. I've got a date field, creation_date, which is not my configured time field for the index pattern. Here's the mapping:

"creation_date": {
  "type": "date",
  "format": "yyyy-MM-dd HH:mm:ss Z"
}

When I create a bar chart with a date histogram on this field, then click on one of the bars, everything seems to work properly. However, when I brush on the chart, I get an error:

jun-20-2017 15-35-16

Is there any way we can mimic the behavior when creating a filter by clicking on the bar?

@nreese
Copy link
Contributor Author

nreese commented Jun 23, 2017

@lukasolson good find with the date parse error. I pushed a change that converts Date to millisecond values to avoid any parse errors on the server. Test it out - it should resolve the issue you found

if (max instanceof Date) {
max = max.getTime();
}
const range = { gte: min, lt: max };
Copy link
Member

Choose a reason for hiding this comment

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

I'm still getting the same error. You'll need to change this line to the following:

const range = { gte: min, lt: max, format: 'epoch_millis' };

Then it will work 😄

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

LGTM!

@nreese nreese merged commit b6e9628 into elastic:master Jun 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:fix v6.0.0-rc1 v6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Date histogram range selection should add filter on visualized date field
6 participants