sapcc / juno

Monorepo for the Juno microfrontend framework, microfrontend apps, design system and component library
Apache License 2.0
5 stars 3 forks source link

[UI-Components] DateTimePicker bugs #563

Closed TilmanHaupt closed 3 months ago

TilmanHaupt commented 3 months ago

After implementing the new DateTimePicker, I encountered the following issues:

franzheidl commented 3 months ago

Hi Tillman đź‘‹

Thanks a lot for your valuable feedback! 🙏

I’ve looked into these:

Error / Invalid Input Message in German

Technically speaking not a bug as in the component doing something it shouldn’t, in fact this is behaviour to be expected. Flatpickr, which we wrap, renders input[type=“number”] elements as inputs for the time, and adds increments as passed by the user or using the default (5 mins). So yes, you are absolutely right, this behaviour is related to the minute increment.

If the time is set or the user inputs or pastes any number that is not a multiple of 5 (or whatever the set increment is), the browser will show these kinds of warnings. These are beyond our control, and will vary depending on the browser used.

We could do our own validation, intercept and alter the user input to something that will positively validate. This is complex, not always possible (since some components we wrap will not allow us to pass the necessary event handlers), and is prone to result in an equally unpleasant user experience due to unexpected response to user keyboard or paste input.

However, I totally agree that showing such errors/messages when just setting the time date and time to the current date is unpleasant and unexpected. ~What we could do is set the default minute increment to 1 to prevent this. We will discuss this.~

For now, we will set the default minuteIncrement to 1.

onChange handler not being fired when changing the time

I could only reproduce not firing onChange-handlers in the “With 24 H Time” story. The handler was fired once, but never again for further changes to the time. We could fix this by explicitly passing a time format that also represents hours and minutes to the DateTimePicker. Interesting insight, a good reminder we should definitively document the need to set a corresponding time format when using enableTime! In all other stories it fired fine when the time changed, at least for me.

We will implement a bit of logic to use a default dateFormat that accomodates date and time in case enableTime is set.

No values shown with default Date and Time story

Not a bug. This sets the values in the Hour and Minute inputs to the values passed, but will not make a selection to the date time picker as the descriptive text states. The goal is to let the user make a selection without a preset value, but have a more comfortable starting points to select a time (Starting with 00:00 would require many clicks to set a somewhat more likely desired time) .

We should probably make this even clearer in the documentation though! We will rename the story and improve the prop description to make the purpose of these options clearer.