Skip to content
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

Timepicker: selecting 12pm returns 24 hrs #105

Closed
aaronfg opened this issue Sep 14, 2021 · 12 comments · Fixed by #110
Closed

Timepicker: selecting 12pm returns 24 hrs #105

aaronfg opened this issue Sep 14, 2021 · 12 comments · Fixed by #110
Labels
bug Something isn't working high priority

Comments

@aaronfg
Copy link

aaronfg commented Sep 14, 2021

Using version 0.4.6.

There seems to be a bug with 12PM and 12AM-- selecting 12pm is returning "24 hours". and 12AM is returning "12 hours".

Using the TimePicker with all defaults, just setting the onDismiss and onConfirm:

<TimePickerModal
   visible={showTimePicker}
   onDismiss={onTimePickerDismissed}
   onConfirm={onTimePicked}     
/>

Picking 12 pm somehow adds an extra 12 hrs as if it is midnight.

Expected vs Actual (buggy) results below.

Expected:

Selected Value
11 AM 11 hours
12 PM 12 hours
1 PM 13 hours

What the Bug Shows:

Selected Value
11 AM 11 hours
12 PM 24 hours
1 PM 13 hours

Is the time picker not using standard 24 hr time?

If so, the hours should match as follows:

Selected Value
1 AM 1 hours
2 AM 2 hours
3 AM 3 hours
4 AM 4 hours
5 AM 5 hours
6 PM 6 hours
7 AM 7 hours
8 PM 8 hours
9 PM 9 hours
10 AM 10 hours
11 PM 11 hours
12 PM 12 hours
1 PM 13 hours
2 PM 14 hours
3 PM 15 hours
4 PM 16 hours
5 PM 17 hours
6 PM 18 hours
7 PM 19 hours
8 PM 20 hours
9 PM 21 hours
10 PM 22 hours
11 PM 23 hours
12 AM 24 hours

The logic for 12AM/PM has been swapped with this time picker.

@YahyaBagia
Copy link

Did you find any solution / workaround for this ?

@RichardLindhout
Copy link
Member

12 PM is 24 hours?

@RichardLindhout
Copy link
Member

12 AM is 12:00

The A stands before the P in the alphabet. AM comes first in the alphabet and therefore also first on a day (Morning) and PM comes last (afternoon/night). 3. PM – Passes Midday.

Or am I missing something?

@aaronfg
Copy link
Author

aaronfg commented Sep 30, 2021

12 PM is 24 hours?

Yeah, that's the bug.

As the library stands right now, if a user selects 12pm in the picker, the value returned is 24.

@RichardLindhout
Copy link
Member

RichardLindhout commented Sep 30, 2021 via email

@aaronfg
Copy link
Author

aaronfg commented Sep 30, 2021

I don’t understand it’s correct behaviour how would you now if it’s not 12 am? If it both returns 12

I don't understand your reply.

Selecting 12:00pm in the picker should return this:

{
   hours: 12,
   minutes: 0,
}

But it's not.

it's returning:

{
   hours: 24,
   minutes: 0,
}

This is just categorically wrong.

24 hours is 12 AM. Your picker is saying that 12pm is somehow 12AM.

I don't know how else to explain this. I put the tables above to show the bug. Just look at the values. You're going from 11 to 24 to 13 in the hours as you choose 11AM, 12PM, and 1PM.

As it is now, your picker is using 24 hour time correctly for all hours except for 12pm and 12AM. Those values are reversed. That is the bug.

UPDATE with Expo example:
Here's an Expo Snack where i'm just logging the hours out when you select the time:

https://snack.expo.dev/OjJhGVl_T

Select 12 PM , hit ok and look at the log. it will show hours: 24
Select 12 AM , hit ok and look at the log. it will show hours: 12.

12pm is not the 24th hour of the day.

@RichardLindhout
Copy link
Member

Ok you're right I didn't grow up with AM/PM that's why I was so confused. This is a serious bug..
Thanks for sticking with me @aaronfg and I'm so sorry for being so stubborn.

@RichardLindhout RichardLindhout added bug Something isn't working high priority labels Sep 30, 2021
@RichardLindhout
Copy link
Member

I'll try to make some time in the evening next week to fix this bug

@aaronfg
Copy link
Author

aaronfg commented Sep 30, 2021

No worries! Thanks for looking into it.

Much appreciated!

@J3j3m
Copy link
Contributor

J3j3m commented Oct 11, 2021

I found the problem, I'll try to make PR by the end of the week.

@J3j3m
Copy link
Contributor

J3j3m commented Oct 16, 2021

Pull Request submitted : Link To PR

RichardLindhout added a commit that referenced this issue Oct 20, 2021
fix(timepicker): selecting 12pm returns 24 hrs
@RichardLindhout
Copy link
Member

Thanks a lot @J3j3m!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working high priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants