-
Notifications
You must be signed in to change notification settings - Fork 486
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
refactor(@clayui/time-picker): rewrites the timer to mimic browser time behavior #2175
Conversation
hey @bryceosterhaus I made some changes here. Ready for a next look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Only question would be tests, do we have any already for time-picker? It might be beneficial to add some in this PR to give a bit of safety.
Yeah, I'm going to add the tests still in that PR and documents. |
hey @bryceosterhaus just a status on this. I needed to modify several rules in the Date Picker so it works fine with Time Picker, I'm having to remove the |
hey @bryceosterhaus, I think it's fine now to take a look, I made some modifications to DatePicker. |
Pull Request Test Coverage Report for Build 3024
💛 - Coveralls |
hey @pat270 we can have some class we can add as select in the input when they are focused, just to try to get closer to the behavior of the native time picker. |
072b8ce
to
9b80a59
Compare
@bryceosterhaus I added here the |
…alues with the DatePicker input and remove timeFormat API
9b80a59
to
5444307
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just one nitpick on the tests, otherwise it looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, feel free to merge unless you have anything else you want to add.
Fixes #2106
I'm not quite sure about some behaviors mainly for the case of the
AM/PM
picker, it does not seem so intuitive. I think there is room for some internal improvements in the code, so leave your thoughts. I've tried at most to mimic the behaviors of the native time picker, so it ends up having lots of use cases.I have not done the tests yet nor the documentation, as soon as we are well with it, I will do.