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

lib: Always use ISO format for date picker manual input #20668

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

martinpitt
Copy link
Member

@martinpitt martinpitt commented Jun 26, 2024

PatternFly's DatePicker's textual date input defaults to ISO format
(YYYY-MM-DD), for a good reason: it's unambiguous, and localized date
input is really hard and brittle.

This is also broken for e.g. en-UK: British date format is DD/MM/YY,
while US date format is MM/DD/YY, but we don't have UI to switch between
English countries.

It was also broken for the manual time setting dialog, where the initial
formatting of the date resulted in a format that the dialog didn't
accept.

So give up, and use the default (i.e. ISO) format and parser. Also drop
the custom time/date format function in serverTime.js, and use our
standard timeformat.dateTime() (which has the same output format).

Fixes #19091

[1] https://www.patternfly.org/components/date-and-time/date-picker


It previously looked like this in English (scheduled shutdown time):
date-main

And now like this, in all languages (it's not an off-by-one, I changed the day to tomorrow):
date-pr

When emptying the input you see the corresponding input helper:
date-pr-template

And for the LOLz, the buggy default dialog value for manually setting the server date on main. With this PR, it's the same ISO format as above:

system-time-bug

I suppose this only didn't get a bug report because nobody actually does that..

When people say "hard times" I didn't realize what they mean.. 😅

@martinpitt martinpitt added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jun 26, 2024
@martinpitt martinpitt requested a review from garrett June 26, 2024 16:40
@martinpitt
Copy link
Member Author

martinpitt commented Jun 26, 2024

Matrix conversation:

17:16:50      pitti │ Garrett: quick gut check: you remember our calendar selector widget, in the timer create and scheduled shutdown dialogs? they also offer you to manually type in the date                                             
17:17:39      pitti │ Garrett: how would you feel like if we stop trying to interpret a locale specific date (what's 03/08/2024? Is it UK or US English?) and just supported ISO format? yyyy-mm-dd; it should be ok for our target audience
17:18:06      pitti │ also, we don't even have an option to select British locale, so it's even broken for them right now (switching month and day)                                                                                         
17:21:12    Garrett │ yeah, sure                                                                                                                                                                                                            
17:21:15    Garrett │ that makes sense                                                                                                                                                                                                      
17:21:32    Garrett │ yeah, we don't have a british or intl. english                                                                                                                                                                        
17:21:38    Garrett │ and that would remove confusion too                                                                                                                                                                                   
17:21:55    Garrett │ and, you're right that our audience is probably familiar with yyyy-mm-dd                                                                                                                                              
17:23:37      pitti │ we've also fixed this part like three times for different languages, it's so brittle                                                                                                                                  
17:23:52    Garrett │ yeah                                                                                                                                                                                                                  
17:23:58      pitti │ Garrett: I'm asking because there's no real browser API for localized date input/parsing, so this is annoyingly hard                                                                                                  

wrt. this PR:

Garrett │ pitti: looks mostly fine. Invalid date format warning should be under the date, not time.

This isn't just a bug, more of a mis-design. If both are invalid, we show it like this:

image

Presumably to avoid the strings becoming too long and not fitting next to each other?

So let's do this as a follow-up, see issue #20669

@martinpitt martinpitt removed the request for review from garrett June 26, 2024 16:47
@martinpitt martinpitt removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jun 26, 2024
PatternFly's DatePicker's textual date input defaults to ISO format
(YYYY-MM-DD), for a good reason: it's unambiguous, and localized date
input is really hard and brittle.

This is also broken for e.g. en-UK: British date format is DD/MM/YY,
while US date format is MM/DD/YY, but we don't have UI to switch between
English countries.

It was also broken for the manual time setting dialog, where the initial
formatting of the date resulted in a format that the dialog didn't
accept.

So give up, and use the default (i.e. ISO) format and parser. Also drop
the custom time/date format function in serverTime.js, and use our
standard `timeformat.dateTime()` (which has the same output format).

Fixes cockpit-project#19091

[1] https://www.patternfly.org/components/date-and-time/date-picker
Nothing uses this any more, and that's the date-fns API which is really
hard to replace (see issue cockpit-project#20653)
Copy link
Member

@jelly jelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@allisonkarlitskaya allisonkarlitskaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup, thanks!

@allisonkarlitskaya allisonkarlitskaya merged commit d5688ab into cockpit-project:main Jun 27, 2024
80 checks passed
@martinpitt martinpitt deleted the timeformat branch June 27, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Invalid date format" when setting clock to "manual" using french language.
3 participants