DateTimePicker: fix date format when switching to 12-hour time format

[amustaque97]

Fixes: #17970

Description

Updated the TimePicker component to show time in MM-DD-YYYY format as mentioned in the docs

How has this been tested?

• Run storybook
• Navigate to TimePicker component
• Under the knob section select 12-hours format
• Verify the format

Screenshots

Types of changes

Bugfix

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

[paaljoachim]

Thank you very much for working on this feature, @amustaque97
Let's see I believe @adamsilverstein and @retrofox both have worked on the date picker.
Perhaps Adam og Damian could take a closer look at this PR.

[ciampo]

Hey @amustaque97 , thank you for looking into this!

You definitely spotted an inconsistency between what is documented and how the component behaves. I'm not sure, though, that the fix proposed here is right one.

The docs for the is12Hour prop mention:

Whether we use a 12-hour clock. With a 12-hour clock, an AM/PM widget is displayed and the time format is assumed to be MM-DD-YYYY.

Judging by how the code was written (including tests written specifically for that condition) and by this sentence, I'm inclined to think that the desired behavior here would be to:

• by default, when is12Hour is false (i.e a 24h format is being used), the date format is DD-MM-YYYY
• when is12Hour is set to true, the date format changes to MM-DD-YYYY

In this perspective, I would say the the behavior of the component is currently the opposite of what we'd like it to be.

My inclination here is to:

• implement the MM-DD-YYYY format when is12Hour is true, and otherwise use the DD-MM-YYYY format when is12Hour is false (it looks like this could be done just by flipping the ternary logic used to compute dayMonthFormat)
• improve the docs for the is12Hour prop to be more explicit, and therefore cause less confusion

[amustaque97]

@ciampo thank you so much for taking time to review my PR 😄 .
How should we go about it now? I once again went through the slack discussion and docs but not able to finalise things.

[ciampo]

@ciampo thank you so much for taking time to review my PR 😄 . How should we go about it now? I once again went through the slack discussion and docs but not able to finalise things.

I was not aware of the slack conversation, where it was also assumed that currently, the bug is caused by the logic being the other way around of what it should be.

It was also discussed that tying the date format (DD-MM-YYYY vs MM-DD-YYYY) to the time format (12 vs 24 hours) is not ideal — and I agree! I personally think that the two formats should be chosen and matched independently from each other.

So, what next?

We could remove the is12Hour check (similarly to what has been done so far in this PR) and introduce a new prop to allow consumers of the component to change the date format — although we'd first need to define a couple of things:

• what would be the default date format? To this question, I'd argue that the default date format should be the same that is intended for the component to have by default today — which is DD-MM-YYYY
• what values should the new prop assume? This is the hard part. I believe that it would be wrong to assume that the only allowed values should be DD-MM-YYYY or MM-DD-YYYY(for example, what about YYYY-MM-DD). We should, therefore, probably have allow a set of possible values to choose from.

A simpler approach would be to merge a quick fix in this PR (switch the logic of the is12Hour check), and tackle the additional of a new prop to format the date in a separate PR.

What do folks think?

[amustaque97]

Thanks for the input 😄 .

what would be the default date format?
DD-MM-YYYY 👍🏻

what values should the new prop assume?
I would personally recommend to allow these types are currently being used in WordPress

To get other people input I can raise this concern in our Wednesday meeting!

[ciampo]

I would personally recommend to allow these types are currently being used in WordPress

What you're referring to are these date formatting settings / parameters. Here's a few personal considerations:

• while it's great to have consistency in our date formats, the format that you suggested is globally used in the PHP language, and not as much in JS.
• we would also need to understand how much work it would require to implement such date formatting logic (also considering that today the component uses moment.js, but this may change in an effort to shave some bytes off the overall library size)

In the light of the above, I would recommend to split the work in 2 parts:

• this PR should focus on fixing the bug (i.e. inverting the is12Hour check). This ensures that the fix is released as soon as possible, and is not delayed by any further discussion around the date format.
• we should open a separate issue to discuss the addition of a "date format" prop. If you'd like to bring the topic up during the next meeting, we'll be able to link to that issue for folks to chime in and discuss

[amustaque97]

I would recommend to split the work in 2 parts

Sure, make sense.

this PR should focus on fixing the bug (i.e. inverting the is12Hour check). This ensures that the fix is released as soon as possible, and is not delayed by any further discussion around the date format.

Please review the PR and let's don't delay it.

we should open a separate issue to discuss the addition of a "date format" prop. If you'd like to bring the topic up during the next meeting, we'll be able to link to that issue for folks to chime in and discuss

Agree with you, I will create a new issue.

[ciampo]

Please review the PR and let's don't delay it.

Sure! As mentioned above, this PR should simply invert the is12Hour check (I've added an inline comment to explain that more explicitely).

We should then:

• adjust the unit tests accordingly
• add a CHANGELOG entry
• potentially add more details to the is12Hour prop description, to add more clarity around the fact that the date format is DD-MM-YYYY when that prop is false

[ciampo]

Agree with you, I will create a new issue.

Thank you! It'd be great if you linked it back here once you've created it :)

[amustaque97]

I have addressed review comments.

[amustaque97]

I have created an issue to introduce new props

[amustaque97]

Please don't review PR. I need a day time to review it myself thoroughly. I'm feeling that I have done something wrong. It's late here (going to bed)! Will check it tomorrow.
Thanks

[amustaque97]

cc @ciampo and @paaljoachim

Feel free to review PR

[ciampo]

Code changes LGTM, the date format is now aligned with the docs

default	is12Hour = true

Just left a couple of minor inline comments, once they're addressed we can merge this PR! Thank you @amustaque97

[ciampo]

Some e2e tests are failing, likely unrelated to the changes in this PR.

@amustaque97 , would you mind rebasing on top of latest trunk? It may help with the e2e tests