react-admin Add components to show only time

This PR adds a brand new component: <TimeInput/>. It behaves much like the <DateTimeInput/>, but only shows the time using an input with type=time.

Along with adding a new component, this PR also adds a new prop showDate to the <DateField/> component. This prop, much like showTime, allows the user to control whether to display the date in the field. When used together with showTime, users can show only the time using toLocaleTimeString. If showTime and showDate are both false, the component throws an error.

Tests and documentation have been updated.

I noticed a lot of the code was copy-pasted between <DateInput/>, <DateTimeInput/> and now <TimeInput/>. Would it be better write the logic in another file and reuse it in each component, or do you want to keep it this way to make understanding each component easier for users?

Closes #7914

Jul 01 '22 09:07 arnaudvergnet

One important thing though, since is is a new feature, could you please rebase your branch and target your PR to the next branch instead of master? Thanks :slightly_smiling_face:

Jul 04 '22 08:07 slax57

Lastly, regarding your question about whether we need to factorize the code between <DateInput/>, <DateTimeInput/> and <TimeInput/> a little more, IMO the amount of duplicated code is still small an manageable, and keeps the components easy to understand. I'd keep them the way they are :wink:

Jul 04 '22 08:07 slax57

One important thing though, since is is a new feature, could you please rebase your branch and target your PR to the next branch instead of master? Thanks slightly_smiling_face

Sorry about that I thought I based it on next already.

Lastly, regarding your question about whether we need to factorize the code between <DateInput/>, <DateTimeInput/> and <TimeInput/> a little more, IMO the amount of duplicated code is still small an manageable, and keeps the components easy to understand. I'd keep them the way they are wink

Ok then I will keep it that way.

Jul 04 '22 09:07 arnaudvergnet

@slax57 everything is fixed. My work was based on next but I forgot to change the target to next as well.

Jul 04 '22 10:07 arnaudvergnet

I made this screencast for this purpose

This is the picker from Edge, so a good idea would be to include both gifs to make sure users understand the appearance differs from browser to browser.

See here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/time#appearance

Jul 06 '22 08:07 arnaudvergnet

I added both gifs in a table to make sure users understand render will change based on the browser.

Jul 07 '22 08:07 arnaudvergnet

@slax57 is everything good for you?

Jul 13 '22 10:07 arnaudvergnet

Sorry for the delay. I just tested it in the storybook through make storybook and ran into a bug, I can not set the value of any TimeInput component with a mac on any browser but Safari (Edge, Firefox, Chrome). Do you experience the same issue?

Jul 26 '22 14:07 antoinefricker

Strange I just tested on Firefox on Linux and it works fine. Under the hood it uses an HTML input component with type="time", which is supported by all major browsers: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/time#browser_compatibility

Jul 26 '22 15:07 arnaudvergnet

I understand it is at least surprising. Here is a screencast on Chrome on which I use TimeInput then DateTimeInput. I will try to get a closer look at it.

https://user-images.githubusercontent.com/102964006/181047539-01280193-6e8f-4e09-b9ec-38ebd8e62754.mov

Jul 26 '22 15:07 antoinefricker

Do you only get this behavior on macos? Because I don't have the hardware so I cannot test.

Jul 26 '22 17:07 arnaudvergnet

Can you open the console to check for errors? Maybe there is an issue with the function toLocaleTimeString on your system.

Jul 27 '22 07:07 arnaudvergnet

Hi Arnaud,

We didn't forget your job but the holidays postponed our feedback. We are coming back to you as soon as we can.

Aug 08 '22 08:08 antoinefricker

We tried to reproduce @septentrion-730n 's issue on several other machines, but none of them had the issue. I believe it is safe to (finally!) merge your PR @arnaudvergnet Again, sorry for the delay, and many thanks again for your contribution! :slightly_smiling_face:

Aug 17 '22 08:08 slax57

We tried to reproduce @septentrion-730n 's issue on several other machines, but none of them had the issue. I believe it is safe to (finally!) merge your PR @arnaudvergnet Again, sorry for the delay, and many thanks again for your contribution! slightly_smiling_face

Believe it or not, actually we did some more testing today and now I do reproduce @septentrion-730n 's issue :sweat_smile:

Anyway, the good news is, we have identified the issue. It actually comes from the parseTime function. @septentrion-730n will push a fix directly onto your branch once it is fully tested. Hopefully this time we can merge your PR for good :grin: Stay tuned

Aug 17 '22 12:08 slax57

@slax57 @septentrion-730n applied the changes to the first commit to keep the history clean.

Aug 23 '22 12:08 arnaudvergnet