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.

Sign up for GitHub

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

Jump to bottom

Timepicker on the right #11980

Merged

nreese merged 12 commits into elastic:master from nreese:timepicker-on-right2

Jun 9, 2017

Contributor

nreese commented May 24, 2017 •

edited

Loading

Fixes #4785

Part the PR involves moving the timepicker quick, relative, and absolute panels from the timepicker directive into separate directive's.

nreese requested review from cjcenizal and stacey-gammon

May 24, 2017 05:15

Contributor

stacey-gammon commented May 24, 2017 •

edited

Loading

Hmm, maybe just because I'm used to it, but it looks a little weird to me right aligned. Also, every other panel we have is left aligned so now this one is different. Ultimately I'll leave it up to the design team though.

cjcenizal previously requested changes

View reviewed changes

Contributor

cjcenizal left a comment

Looks great man! I think this is a much better UX. It's much easier/faster to change the time now. I just had some requests for changing some classes to use UI Framework components.

src/ui/public/timepicker/absolute_panel/kbn_timepicker_absolute_panel.html Outdated

    
                      </div>

                    </div>

                    <input type="text" required class="form-control" input-datetime="{{format}}" ng-model="absolute.from">

Contributor

cjcenizal May 25, 2017

Can we change this class to kuiTextInput fullWidth?

src/ui/public/timepicker/absolute_panel/kbn_timepicker_absolute_panel.html Outdated

    
                    </div>

                    <span ng-show="!absolute.to"><em>Invalid Date</em></span>

                    <input type="text" required class="form-control" input-datetime="{{format}}" ng-model="absolute.to">

Contributor

cjcenizal May 25, 2017

Same here.

src/ui/public/timepicker/relative_panel/kbn_timepicker_relative_panel.html Outdated

    
                          greater-than="-1"

                          min="0"

                          type="number"

                          class="form-control fullWidth">

Contributor

cjcenizal May 25, 2017

Can we change this class to kuiTextInput fullWidth?

src/ui/public/timepicker/relative_panel/kbn_timepicker_relative_panel.html Outdated

    
                    </div>

                    <div class="kuiFieldGroup kuiVerticalRhythmSmall">

                      <div class="kuiFieldGroupSection kuiFieldGroupSection--wide" ng-class="{ 'has-error': checkRelative() }">

Contributor

cjcenizal May 25, 2017

Instead of the ng-class here, can we put it on the input instead?

<input
  ng-class="{ 'kuiTextInput-isInvalid' : checkRelative() }"
  etc...
>

You will need to rebase master to make this class available though, since I just added it.

Contributor Author

nreese Jun 2, 2017

Putting ng-class inside the input causes some problems. When you are editing the input, it is styled for having focus. As soon as the user puts in a value that causes checkRelative to flag an error than the error message appears but the input styling does not change. The input styling only shows the red border after the input looses focus.

src/ui/public/timepicker/relative_panel/kbn_timepicker_relative_panel.html Outdated

    
                          ng-model="relative.from.unit"

                          ng-options="opt.value as opt.text for opt in relativeOptions"

                          ng-change="formatRelative({key:'from'})"

                          class="form-control fullWidth">

Contributor

cjcenizal May 25, 2017

Can we change this class to kuiSelect fullWidth?

src/ui/public/timepicker/relative_panel/kbn_timepicker_relative_panel.html Outdated

    
                          class="form-control fullWidth">

                      </div>

                      <div class="kuiFieldGroupSection" ng-class="{ 'has-error': checkRelative() }">

Contributor

cjcenizal May 25, 2017

Can we move this ng-class to the select, but assign kuiSelect-isInvalid instead of has-error?

Contributor Author

nreese Jun 2, 2017

Setting kuiSelect-isInvalid on the select does not visually effect the select styling.

src/ui/public/timepicker/relative_panel/kbn_timepicker_relative_panel.html Outdated

    
                    </div>

                    <div class="kuiVerticalRhythmSmall">

                      <label>

Contributor

cjcenizal May 25, 2017

Can we use the kuiCheckBox class here, with the relevant markup for making sure the label is clickable. In the UI Framework, the example looks like this:

<label class="kuiCheckBoxLabel">
  <input
    class="kuiCheckBox"
    type="checkbox"
  >
  <span class="kuiCheckBoxLabel__text">
    With clickable label
  </span>
</label>

src/ui/public/timepicker/relative_panel/kbn_timepicker_relative_panel.html Outdated

    
                          greater-than="-1"

                          min="0"

                          type="number"

                          class="form-control fullWidth">

Contributor

cjcenizal May 25, 2017

Same comments as above for this input

src/ui/public/timepicker/relative_panel/kbn_timepicker_relative_panel.html Outdated

    
                          ng-model="relative.to.unit"

                          ng-options="opt.value as opt.text for opt in relativeOptions"

                          ng-change="formatRelative({key:'to'})"

                          class="form-control fullWidth">

Contributor

cjcenizal May 25, 2017

Same comments as above for this select

src/ui/public/timepicker/relative_panel/kbn_timepicker_relative_panel.html Outdated

    
                    </div>

                    <div class="kuiVerticalRhythmSmall">

                      <label>

Contributor

cjcenizal May 25, 2017

Same comments as above for this checkbox

Contributor

stacey-gammon commented May 25, 2017

You may need to do some trimming of whitespace to get the tests to pass:

Chrome 58.0.3029 (Linux 0.0.0) timepicker directive relative mode has a preview of the "from" input FAILED
	Error: expected '\n          January 1st 2014, 05:51:06.666\n        ' to equal 'January 1st 2014, 05:51:06.666'
	    at Assertion.assert (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=1:437228:14)
	    at Assertion.be.Assertion.equal (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=1:437348:11)
	    at Assertion.(anonymous function) [as be] (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=1:437201:25)
	    at Context.<anonymous> (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=1:456778:49)
Chrome 58.0.3029 (Linux 0.0.0): Executed 146 of 829 (1 FAILED) (0 secs / 1.016 secs)
Chrome 58.0.3029 (Linux 0.0.0) timepicker directive relative mode has a disabled "to" field that contains "Now" FAILED
	Error: expected '\n          Now\n        ' to equal 'Now'

Other than that and @cjcenizal's comments, looks good! Love the refactoring & clean up!

nreese force-pushed the timepicker-on-right2 branch 2 times, most recently from b4a0a9a to c8f5de3 Compare

June 2, 2017 21:07

nreese dismissed cjcenizal’s stale review

June 5, 2017 14:23

Made changes but github did not reflect that all comments have been addressed

cjcenizal approved these changes

View reviewed changes

Contributor

cjcenizal left a comment

Just had two minor suggestions and then it LGTM!

src/ui/public/timepicker/timepicker.less Outdated

    
            @@ -8,7 +16,30 @@
          
                }

                .kbn-timepicker-section {

                  float: left;

                  padding: 0px 15px;

                  padding-right: 15px;

Contributor

cjcenizal Jun 9, 2017

We can make each column the same width, and also remove the unnecessary padding from the last column by changing these styles:

  .kbn-timepicker-section {
    float: left;
    width: 25%;

    & + .kbn-timepicker-section {
      padding-left: 15px;
    }
  }

src/ui/public/timepicker/timepicker.less Outdated

    
                  display: flex;

                  align-items: center;

                }

                .nav > li > a {

Contributor

cjcenizal Jun 9, 2017

Can we just use a class here instead? E.g. .kbn-timerpicker-nav-button?

nreese and others added 11 commits

June 9, 2017 09:11


          quick panel moved to directive

2fd1b01


          move relative to directive

f663f67


          move absolute to directive

7e0534e


          move stuff from timepicker directive to new directives

bc596d0


          move timepicker to right with flexbox

ae1d7a7


          remove left padding for kbn-timepicker-section

01d2f74


          merge in timepicker.html changes from - Improve accessibility of the …

4160e97

…Datepicker. (elastic#11753)


          Adjust markup and styles so that the Quick, Relative, and Absolute la…

2c6ed1e

…youts all occupy the same bounds (#1)

* Adjust markup and styles so that the Quick, Relative, and Absolute layouts all occupy the same bounds, so the content doesn't jump around as you switch modes..

* Make kbn-timepicker-content responsive so that it stacks content on narrower screens.

* Combine Relative input and select fields using kuiFieldGroup.

* Make Timepicker submit button wider.

* Align button on right.


          migrate inputs from form-control to kui classes

c125ee2


          update generated dist/ui_framework.css

6f1e4fb


          fix broken tests

e7c9861

nreese force-pushed the timepicker-on-right2 branch from a262ca3 to e7c9861 Compare

June 9, 2017 15:12


          create kbn-timerpicker-nav-button class and update kbn-timepicker-sec…

27b413d

…tion to make each column the same width

Member

lukasolson commented Jun 9, 2017

Thanks for taking this on, @nreese! Awesome to see this broken up into separate modules and cleaned up 😄

nreese merged commit 5c075f2 into elastic:master

nreese added v6.0.0-alpha3 v6.0.0 and removed v6.0.0-alpha3 labels

jimgoodwin added the release_note:enhancement label

jimgoodwin added the :Discovery label

jimgoodwin added the v6.0.0-rc1 label

timroes mentioned this pull request

Fix broken date range look #14837

Merged

ppf2 mentioned this pull request

Lot of empty space to the left of the timepicker at certain resolutions #15561

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_note:enhancement v6.0.0-rc1 v6.0.0