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

Add components to show only time #7917

Merged
merged 4 commits into from
Aug 23, 2022
Merged

Conversation

arnaudvergnet
Copy link
Contributor

@arnaudvergnet arnaudvergnet commented Jul 1, 2022

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

Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Great contribution, thank you so much! 🙂

docs/TimeInput.md Show resolved Hide resolved
docs/navigation.html Outdated Show resolved Hide resolved
packages/ra-ui-materialui/src/input/TimeInput.tsx Outdated Show resolved Hide resolved
@slax57
Copy link
Contributor

slax57 commented Jul 4, 2022

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 🙂

@slax57
Copy link
Contributor

slax57 commented Jul 4, 2022

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 😉

@arnaudvergnet
Copy link
Contributor Author

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 , and 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.

@arnaudvergnet
Copy link
Contributor Author

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

Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Sorry I took so long to review, I was too busy to do it before.

I have one last request, and then we will be good to merge this! 🙂

I just think it would be better to showcase the time picker from the browser, which demonstrates a little better imo the benefits of this component.

I made this screencast for this purpose:
time-input

Would you mind using this one in the docs?
Thanks

@arnaudvergnet
Copy link
Contributor Author

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

@arnaudvergnet
Copy link
Contributor Author

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

@arnaudvergnet
Copy link
Contributor Author

@slax57 is everything good for you?

@antoinefricker
Copy link
Contributor

antoinefricker commented Jul 26, 2022

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?

@arnaudvergnet
Copy link
Contributor Author

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

@antoinefricker
Copy link
Contributor

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.

Enregistrement.de.l.ecran.2022-07-26.a.17.28.47.mov

@arnaudvergnet
Copy link
Contributor Author

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

@arnaudvergnet
Copy link
Contributor Author

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

@antoinefricker
Copy link
Contributor

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.

@slax57
Copy link
Contributor

slax57 commented Aug 17, 2022

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! 🙂

@slax57
Copy link
Contributor

slax57 commented Aug 17, 2022

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 😅

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 😁
Stay tuned

@arnaudvergnet
Copy link
Contributor Author

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

Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Thank you so much! 🙂

@slax57 slax57 added this to the 4.3 milestone Aug 23, 2022
@slax57 slax57 merged commit f430559 into marmelab:next Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add time field and input
3 participants