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

TimePicker components should have option to disable date field #52734

Closed
Tropicalista opened this issue Jul 18, 2023 · 6 comments · Fixed by #63145
Closed

TimePicker components should have option to disable date field #52734

Tropicalista opened this issue Jul 18, 2023 · 6 comments · Fixed by #63145
Assignees
Labels
[Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.

Comments

@Tropicalista
Copy link

At the moment there is no component to display a time picker that only show time.

The timepicker component has no optio to disable the date field. So it's mostly a light version of datetimepicker.

@Tropicalista Tropicalista changed the title TimePicker components should have option to disable date fied TimePicker components should have option to disable date field Jul 18, 2023
@t-hamano
Copy link
Contributor

This suggestion makes sense to me. My feeling is that TimePicker should not inherently have the date control.

The current implementation of DateTimePicker, DatePicker, TimePicker and the specifications I expect are as follows:

Actual

Component Has date input? Has calendar? Has time input?
DateTimePicker
DatePicker
TimePicker

Expected

Component Has date input? Has calendar? Has time input?
DateTimePicker
DatePicker
TimePicker

@t-hamano t-hamano added the [Package] Components /packages/components label Jul 19, 2023
@jordesign jordesign added the [Type] Enhancement A suggestion for improvement. label Jul 19, 2023
@mirka
Copy link
Member

mirka commented Aug 16, 2023

I think the complexity here is that TimePicker is built for manipulating full Date values like 2023-08-17T08:41:00. So it isn't just a matter of visually hiding the inputs for the date part, unless you're ok with passing half-fake Date values to currentTime and getting a half-fake value back from the onChange.

@bogiii
Copy link
Contributor

bogiii commented Mar 20, 2024

I absolutely agree with the proposed expected table specification from the comment above.

I think the complexity here is that TimePicker is built for manipulating full Date values like 2023-08-17T08:41:00
Unless you're ok with passing half-fake Date values to currentTime and getting a half-fake value

Changing a format is not an option since it sounds like a breaking change.

So, if we want to support hiding the date part, the only option is to keep the same component configuration with the full format and provide an additional boolean property.

Here is the draft PR: #60034

@mirka, what do you think?

@bogiii
Copy link
Contributor

bogiii commented Mar 22, 2024

I've investigated the other component libraries and watched how they return the time picker value. Most of them return it in the full-date format, with the current date if it's not set, which is also our case.

From my perspective, it isn't a half-fake date string and a blocker for further component improvements.

If this still doesn't sound like a possible solution, I would recommend creating a new separate Time picker without a date part. I can open a feature request, and then we can shape the details.
For example:

  • entry params: currentTime, 12hourFormat, showMinutes, showSeconds
  • onChange time format
  • what elements to support: hours, minutes, seconds, 12HoursFormat

Once we build the component, we can incorporate it in the current time picker with the date component, so we maintain only one Time component.

What do you think?

@mirka
Copy link
Member

mirka commented Apr 4, 2024

@bogiii Apologies for the late reply!

To add some context for this thread: I wasn't sure if I was being too idealistic about the Date string issue, so I discussed it with @tyxla for a second opinion, and we agreed that while it isn't a hard blocker, it is indeed a code smell that should be avoided if possible. And we thought about a possible way forward, which is basically what @bogiii is suggesting:

  1. Extract just the time input component, e.g. as TimeInput in a separate time-input.tsx file.
  2. The data type of the value should be something like { hours: number; minutes: number; }, which is truthful to the component and easy to parse in/out of Date objects or simple time strings in whatever format.
  3. Incorporate this TimeInput component back into the TimePicker.
  4. Publicly export TimeInput as a subcomponent of TimePicker, e.g. called TimePicker.TimeInput.

As for the props, let's keep it simple and stick to the API surface of TimePicker for now — so just currentTime, is12Hour, and onChange.

@bogiii
Copy link
Contributor

bogiii commented Apr 12, 2024

Hey @mirka, thanks for the reply.

I followed your instructions and created the TimeInput component. The PR is ready for review: #60613

@mirka mirka added the [Status] In Progress Tracking issues with work in progress label Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants