-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(timepicker): switch to PM when hour > 12 #1684
Conversation
@@ -224,7 +224,11 @@ export class NgbTimepicker implements ControlValueAccessor, | |||
updateHour(newVal: string) { | |||
const isPM = this.model.hour >= 12; | |||
const enteredHour = toInteger(newVal); | |||
this.model.updateHour((this.meridian ? enteredHour % 12 : enteredHour) + (this.meridian && isPM ? 12 : 0)); | |||
if (this.meridian && (isPM && enteredHour < 12 || !isPM && enteredHour === 12)) { | |||
this.model.updateHour(enteredHour + 12); |
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.
What would happen if I enter, say 30? I don't think that this logic here is enough...
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.
if you enter 30
it will try to set it as an hour without any corrections and updateHour
will do % 24
, so the result will be 6
, so it will switch to AM.
@dmytroyarmak thnx for the PR. I can see how it would solve the existing issue, but IMO if we want to fix it one and for all we need to decide what happens if a user enters any number > 12 for the meridian on and AM / PM being selected. As such we should have tests for all those cases. Would you be willing to work more on this PR and add more tests that prove proper behaviour in all corner cases? |
@pkozlowski-opensource yeah, I can do more work on this PR
So, in general, if value is <= 12 we store it as a correct value in current meridian by doing needed corrections to transform it to 24h format. But if value is > 12, we just save its remainder of division by 24 as a value in 24h format. @pkozlowski-opensource If this logic is correct I can cover not-covered already cases with unit tests. |
@dmytroyarmak Yup, your reasoning looks fine! If you could please add more tests for the listed cases it would be great. I will merge your PR as soon as we've got tests covering all the cases. Thnx so much for helping it, this is much appreciated! 👍 |
@pkozlowski-opensource I found that there were already some test about handling different values in meridian, I've moved them to 'meridian' section and added missed cases |
Fixes #1680