Skip to content

Conversation

@kobelb
Copy link
Contributor

@kobelb kobelb commented Oct 6, 2016

Closes #8474

@thomasneirynck thomasneirynck self-assigned this Oct 6, 2016
@kobelb kobelb added the review label Oct 6, 2016
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.

Some minor suggestions, otherwise LGTM.

class="form-control"
name="interval"
min="0"
input-whole-number
Copy link

@stacey-gammon stacey-gammon Oct 6, 2016

Choose a reason for hiding this comment

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

I think this might be the only reference to input-whole-number - can you delete the file and corresponding test file?

import angular from 'angular';
import expect from 'expect.js';
import ngMock from 'ng_mock';
import 'ui/directives/input_whole_number';

Choose a reason for hiding this comment

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

import input_number and not input_whole_number? Interesting, I wonder how this is working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WHOOPPPSS. Thanks for catching that. Not sure how it's working either

restrict: 'A',
require: 'ngModel',
link: function ($scope, $elem, attrs, ngModel) {
ngModel.$parsers.push(checkNumber);

Choose a reason for hiding this comment

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

Just my preference but I would first define the function, then use it. Not sure if there is an official style for this kind of thing. Up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure regarding official style either, I was just emulating how it was done in the input_whole_number.js

Choose a reason for hiding this comment

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

consistency sounds like a good reason to me :)

@thomasneirynck
Copy link
Contributor

LGTM

@epixa epixa changed the title Modifying the histograms interval to support decimals Support decimals in histogram interval Oct 8, 2016
@epixa
Copy link
Contributor

epixa commented Oct 8, 2016

@kobelb I updated the title to reflect the change itself rather than describing the action, which is more appropriate for release notes and such.

@kobelb kobelb merged commit 6000a90 into elastic:master Oct 10, 2016
elastic-jasper added a commit that referenced this pull request Oct 10, 2016
---------

**Commit 1:**
Modifying the histograms interval to support decimals

* Original sha: da04e10
* Authored by = <[email protected]> on 2016-10-05T20:46:52Z

**Commit 2:**
Removing the input-whole-number, no longer used.

* Original sha: cccb370
* Authored by = <[email protected]> on 2016-10-06T17:29:03Z
kobelb added a commit that referenced this pull request Oct 10, 2016
[backport] PR #8566 to 5.x - Support decimals in histogram interval
@epixa epixa added v5.1.1 and removed v5.1.0 labels Dec 8, 2016
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
---------

**Commit 1:**
Modifying the histograms interval to support decimals

* Original sha: 189c60d45010eb843dc573863aaf0fb85fbaefc6 [formerly da04e10]
* Authored by = <[email protected]> on 2016-10-05T20:46:52Z

**Commit 2:**
Removing the input-whole-number, no longer used.

* Original sha: 6217f3d8bf453f8431f90f5f5b3acc4defaf6ad6 [formerly cccb370]
* Authored by = <[email protected]> on 2016-10-06T17:29:03Z


Former-commit-id: 2172489
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
[backport] PR elastic#8566 to 5.x - Support decimals in histogram interval

Former-commit-id: 05635eb
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.

5 participants