Conversation
|
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
left a comment
There was a problem hiding this 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.
There was a problem hiding this comment.
Can we change this class to kuiTextInput fullWidth?
There was a problem hiding this comment.
Can we change this class to kuiTextInput fullWidth?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can we change this class to kuiSelect fullWidth?
There was a problem hiding this comment.
Can we move this ng-class to the select, but assign kuiSelect-isInvalid instead of has-error?
There was a problem hiding this comment.
Setting kuiSelect-isInvalid on the select does not visually effect the select styling.
There was a problem hiding this comment.
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>There was a problem hiding this comment.
Same comments as above for this input
There was a problem hiding this comment.
Same comments as above for this select
There was a problem hiding this comment.
Same comments as above for this checkbox
|
You may need to do some trimming of whitespace to get the tests to pass: Other than that and @cjcenizal's comments, looks good! Love the refactoring & clean up! |
b4a0a9a to
c8f5de3
Compare
Made changes but github did not reflect that all comments have been addressed
cjcenizal
left a comment
There was a problem hiding this comment.
Just had two minor suggestions and then it LGTM!
There was a problem hiding this comment.
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;
}
}
There was a problem hiding this comment.
Can we just use a class here instead? E.g. .kbn-timerpicker-nav-button?
…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.
a262ca3 to
e7c9861
Compare
…tion to make each column the same width
|
Thanks for taking this on, @nreese! Awesome to see this broken up into separate modules and cleaned up 😄 |
Fixes #4785
Part the PR involves moving the timepicker quick, relative, and absolute panels from the timepicker directive into separate directive's.