-
Notifications
You must be signed in to change notification settings - Fork 9
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
Time setting seems to default to 24h #850
Comments
Example test failure here.
The test started failing after production release 0406. Before the release 12HR time was used (as expected by the test). I'm guessing maybe this is the change that caused the date/time to now appear as 24hr time (by default)? @devmount Was the default always 24 hour format but your PR above fixed it so that the defaults were applied? In which case I will update the production sanity E2E test to expect 24 hour format now instead. :) |
I guess I'm to blame here 😅 I implemented the general settings store in #828 which set 24h as default iirc and nobody complained 😂 |
@rwood-moz Yes, in my understanding it was like this. If you rather want h12 as default, we can implement that too. |
@devmount No blame, just trying to figure out what happened :) For the default time format (12 or 24hr) whatever you and @MelissaAutumn prefer works for me! If it's 24 hr I can update the tests easily. :) |
@MelissaAutumn I think you approved the h24 default? Any opinion on this? |
We should probably be consistent with default settings between services. Most of the team is 12 hour time based, but dunno about user stats. cc @malini @ryanjjung @radishmouse @aaspinwall for any input / suggestions. |
I don't know either, honestly. I'd default to 12hr for users and they can swap to 24hrs. My guess is that people used to 12hrs would be immediately confused by seeing 18:00 and might think it an error, but someone used to 24 hours would see 6PM and think "ugh, gotta switch to 24 hour mode". |
Alright, I'll switch this to 12 hr default. |
I'm guessing this is related to the store changes, but it seems like it at least breaks sanity tests.
cc @rwood-moz @devmount
The text was updated successfully, but these errors were encountered: