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

bug: ion-datetime is broken/fails when the current value is not defined, the min value is in the future and time format is 12-hour #25296

Closed
4 of 7 tasks
ryaa opened this issue May 17, 2022 · 7 comments
Labels

Comments

@ryaa
Copy link

ryaa commented May 17, 2022

Prerequisites

Ionic Framework Version

  • v4.x
  • v5.x
  • v6.x
  • Nightly

Current Behavior

Datetime control show empty time popover and when it is dismissed an error occurs which makes ion-datetime unusable.
Note that the value provided to the control is null and the min property must be in the future. If the min property is in the past then everything works fine.

Expected Behavior

Time part needs to be displayed correctly and no error when it is dismissed/closed

Steps to Reproduce

  1. open this link https://ionic6-angular13-qkrphu.stackblitz.io/
  2. tap on the button
  3. tap on the time control
    Note that the popover opened is empty - see below
    Screen Shot 2022-05-17 at 07 41 21
  4. tap outside of the popover to dismiss it
    Note that an error is triggered/thrown - see below
    Screen Shot 2022-05-17 at 07 41 38

You can also see the problem here https://stackblitz.com/edit/ionic6-angular13-qkrphu?file=src%2Fapp%2Fapp.component.ts

Code Reproduction URL

https://stackblitz.com/edit/ionic6-angular13-qkrphu?file=src%2Fapp%2Fapp.component.ts

Ionic Info

See https://stackblitz.com/edit/ionic6-angular13-qkrphu?file=src%2Fapp%2Fapp.component.ts

Additional Information

This seems to be related to #22756 which has been reported to be fixed.

@ionitron-bot ionitron-bot bot added the triage label May 17, 2022
@liamdebeasi liamdebeasi self-assigned this May 17, 2022
@liamdebeasi
Copy link
Contributor

Thanks for the issue. Things seem to be working as intended here, though the UX can be improved.

In your demo, the current selected date is May 17th (today's date). However, the minimum selectable date is May 20th. This means that there are no times on May 17th that you should be able to select. As a result, no values will appear in the time picker.

We are tracking UX improvements for this in #24881. One idea is to have the initial date always be in bounds, even if that means the initial date is not today's date.

Does this align with what you are seeing on your end?

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label May 17, 2022
@liamdebeasi liamdebeasi removed their assignment May 17, 2022
@ionitron-bot ionitron-bot bot removed the triage label May 17, 2022
@ryaa
Copy link
Author

ryaa commented May 17, 2022

@liamdebeasi thank you very much for a very quick response on this. Please see my comments below

Does this align with what you are seeing on your end?

not really. please see below

In your demo, the current selected date is May 17th (today's date).

There is no selected date really since the datetime is provided null as a value. Is it possible to prevent any date to be pre-selected in the datetime component? or may be, the datetime component should pre-select the default date (if none is provided) taking into account the min parameter (so that it is the same or greater then the min date)

This means that there are no times on May 17th that you should be able to select.

This implies that the time part/control should be disabled (and show empty control?) and prevent the user from interacting with it until the user selects the date which is equal or greater than the min date.

As a result, no values will appear in the time picker.

This is part of the problem (and I think that this behavior will confuse the end user). The other problem is that the datetime throw an error (see the console) which, I believe, very good reason to consider this as a bug and not as potential UX improvement.

To summarize:
I think that the datetime component should:

  1. allow to configure to disable date pre-selection (opt-in?)
  2. the default pre-selected date must take into account the min component property value. There is one corner case when the min date is the same as today date but its time is less than the current time. In this case the time part can be enabled but the min selectable time must still take into account the the min component property value
  3. time part control must be disabled if the date and time selected is below the min component property value (this does not apply if the option 2 above is supported)
  4. there should be no error throw in any use case

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels May 17, 2022
@liamdebeasi
Copy link
Contributor

liamdebeasi commented May 17, 2022

There is no selected date really since the datetime is provided null as a value. Is it possible to prevent any date to be pre-selected in the datetime component? or may be, the datetime component should pre-select the default date (if none is provided) taking into account the min parameter (so that it is the same or greater then the min date)

This is what I was referring to in my previous comment. Right now the default active date in the datetime is today's date. If you provide a value, then the active date will use that instead. There is no way to prevent this "pre selecting" behavior at the moment.

This implies that the time part/control should be disabled (and show empty control?) and prevent the user from interacting with it until the user selects the date which is equal or greater than the min date.

The component's current flow in this scenario needs more design from our end. Having a disabled/empty control seems confusing, and we think it would be better to prevent the user from getting into this state in the first place. This is why I suggested having the pre-selected date always default to an active date that is in range (even if the active date is not today).

This is part of the problem (and I think that this behavior will confuse the end user). The other problem is that the datetime throw an error (see the console) which, I believe, very good reason to consider this as a bug and not as potential UX improvement.

I agree that it is confusing. This is why I mentioned #24881 as it is tracking the UX improvements we are looking to do for scenarios like this. The current implementation of the picker requires that there be items to select. Since there are no items to select in the sample app, an error is thrown. It is possible that we will revise this behavior in the future, but at the moment this is the intended behavior. As I mentioned previously, we feel that users should never be put into this state in the first place.


Ensuring that the pre-selected date is in range (currently being tracked in #24881) should resolve the following issues:

  1. The time picker does not have any items to select.
  2. An error is thrown since there are no items to select in the picker.

Does that clarify things?

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label May 17, 2022
@ionitron-bot ionitron-bot bot removed the triage label May 17, 2022
@liamdebeasi
Copy link
Contributor

liamdebeasi commented May 17, 2022

A couple other cases that could come up:

  1. The max date is before than the min date.
  2. The max and min dates are the same.

I would argue that these are incorrect usages, but it would probably be good to have a console warning. Otherwise, users will get into the same situation described here.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels May 17, 2022
@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label May 17, 2022
@ionitron-bot ionitron-bot bot removed the triage label May 17, 2022
@ryaa
Copy link
Author

ryaa commented May 17, 2022

Ensuring that the pre-selected date is in range (currently being tracked in https://github.com/ionic-team/ionic-> framework/issues/24881) should resolve the following issues:

The time picker does not have any items to select.
An error is thrown since there are no items to select in the picker.
Does that clarify things?

Yes. This makes sense however there is still an issue. I have to set some value (to make sure that pre-selected value is in the range) to the datetime component which I don't want to. The value should be unset/null unless the user explicitly set it.

There is one more minor thing as well. The current and the min date are on the same date but the min time is greater than the current time. The datetime control will show the below (my current time is 5:34 PM and the min time is 7PM of the same date)
Screen Shot 2022-05-17 at 17 34 15
However, when the user makes any changes in the time part of the component it will update the displayed time and no error is thrown.

Thank you very much for all the detailed explanations.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels May 17, 2022
@liamdebeasi
Copy link
Contributor

The value should be unset/null unless the user explicitly set it.

I recommend opening a separate feature request for this. The current behavior was implemented to align with the native iOS and Android date pickers, so having a separate discussion on this would help us make a more informed decision about which path to take.

There is one more minor thing as well. The current and the min date are on the same date but the min time is greater than the current time. The datetime control will show the below (my current time is 5:34 PM and the min time is 7PM of the same date)

This is the same problem as #24881. Ensuring that the value is always in bounds (or adding a console.warn to inform the developer) should help here.

I am going to close this in favor of #24881. You can track any progress on that thread. Thanks for all the helpful context!

@ionitron-bot
Copy link

ionitron-bot bot commented Jun 17, 2022

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Jun 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants