Skip to content

Conversation

@nreese
Copy link
Contributor

@nreese nreese commented Jun 4, 2018

Part of moving Kibana's global timepicker to react will require migrating the timefilte from an angular service to a singleton. In order to avoid making a giant PR, this PR is just one phase in that migration. This PR removes Private from timefilter/lib and migrates mocha tests to jest.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese
Copy link
Contributor Author

nreese commented Jun 4, 2018

jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link

@stacey-gammon stacey-gammon left a comment

Choose a reason for hiding this comment

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

LGTM. code review + pulled it down and played around with it. Visualizations updated with a change in time. Manually stepped through to investigate whether the logic change would have an impact and doesn't seem like it.

return true;
}
} else {
return !_.isEqual(rangeA, rangeB);

Choose a reason for hiding this comment

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

This technically is different than the old code (I think anyway) because angular.equals doesn't care about variables on an object that start with $. But playing around with it, it doesn't seem to ever matter, so I think it's okay to change this. It actually seems pretty weird to ever fall in here anyway - only seems to be the case if either rangeA or rangeB are undefined.

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

Code looks good, pulled it down and seems things are working great. Updating the time via the time picker, modifying the URL, dragging & clicking on a vis all work as expected. 👍

@nreese nreese force-pushed the timefilter_phase1 branch from 6fd47fe to 2739e0c Compare June 13, 2018 01:15
@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese
Copy link
Contributor Author

nreese commented Jun 13, 2018

jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese nreese merged commit 5b05c4c into elastic:master Jun 13, 2018
nreese added a commit to nreese/kibana that referenced this pull request Jun 13, 2018
maryia-lapata pushed a commit to maryia-lapata/kibana that referenced this pull request Jun 25, 2018
@nreese nreese deleted the timefilter_phase1 branch July 13, 2018 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants