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

DatePicker support for disabling arbitrary dates #2867

Open
11 tasks
rolfsmeds opened this issue Mar 28, 2022 · 34 comments
Open
11 tasks

DatePicker support for disabling arbitrary dates #2867

rolfsmeds opened this issue Mar 28, 2022 · 34 comments
Labels
acceptance criteria used for the acceptance criteria checklist DS Design System feature (e.g. component)

Comments

@rolfsmeds
Copy link
Collaborator

rolfsmeds commented Mar 28, 2022

Description

A mechanism for dynamically disabling arbitrary dates in the date picker overlay and automatically invalidating entering them manually.

Use cases

As a user
I want the date picker to show me which dates are unavailable, and prevent me from inadvertently picking or entering such a date.
So that I can avoid entering unavailable dates

Relation to min/max

While this feature partially overlaps the functionality provided by the existing min/max feature, this would give developers more granular control over available dates (allowing them to e.g. disable certain weekdays or individual dates), rather than just enabling a range of dates.

API proposal

An API similar to ItemEnabledProvider in CheckboxGroup and RadioButtonGroup would probably make sense here, i.e. something along the lines of
public void setDateEnabledProvider​(SerializablePredicate<LocalDate> dateEnabledProvider)
that would be used like

datepicker.setDateEnabledProvider(date->{
  return !date.getDayOfWeek().equals(DayOfWeek.SUNDAY);
});

Acceptance criteria

  • Disabled dates rendered as such in the picker calendar (same as those outside min/max range)
  • Manual entry of a disabled date invalidates field
  • Min/max range has priority, i.e. if min/max range is also provided, anything outside it will be disabled even if the enabledProvider returns true.
  • Dates disabled this way shall be keyboard-focusable (as opposed to dates outside the min/max range, which are, and shall continue to be, non-focusable)
  • Java and WC APIs for providing the function

General criteria

  • APIs reviewed
  • Performance (should not have noticeable effect on datepicker performance)
  • UX/DX tests in Alpha
  • Documentation: (add new subsection under the Validation section in Date Picker docs)
  • How to test?
  • Limitations:
@rolfsmeds rolfsmeds added acceptance criteria used for the acceptance criteria checklist DS Design System feature (e.g. component) labels Mar 28, 2022
@jouni
Copy link
Member

jouni commented Mar 28, 2022

Re: min/max: I suppose we could migrate those to internally use this feature? Or should those be marked as deprecated and instruct users to use this new feature instead?

@knoobie
Copy link
Contributor

knoobie commented Mar 28, 2022

I would suggest to have min/max stay like they are at the moment. Now the web component can check if something is > max without additional server round-trips.. with the proposed java API the web component has to ask the server again.

@jouni
Copy link
Member

jouni commented Mar 28, 2022

Hmm, right. I was only considering the client-side use. But I still feel min/max should be possible to internally use this new feature instead of being a separate implementation.

@rolfsmeds
Copy link
Collaborator Author

Neither for or against refactoring min/max to be based on this, but I do think it's meaningful to provide the min/max API after this is in, as it's a simpler, dedicated API for what is probably a more common use case than disabling arbitrary dates.

@jouni
Copy link
Member

jouni commented Mar 29, 2022

I worry that an additional min/max API requires developers to think how these APIs interact, and that less API would be easier to maintain when there would not be the need to orchestrate them together. That’s all.

I assume min/max would always override this other way of disabling dates, and I feel that’s how the majority will assume it works. But there might be some who think they can re-enable a particular day outside the min–max range by returning true from setDateEnabledProvider(). A thing to mention in the documentation, I believe.

Min/max is surely more convenient than something like this, I admit that:

datePicker.isDateAvailable( date => Date.UTC(2022, 0) <= date && date <= Date.UTC(2022, 11) );

or

render() {
  return html`<vaadin-date-picker is-date-available="${ date => Date.UTC(2022, 0) <= date && date <= Date.UTC(2022, 11) }">`;
}

compared to

<vaadin-date-picker min="2022-01" max="2022-12">

@web-padawan
Copy link
Member

I've just realised that implementing the proposed Java API might be problematic. The server side check needs to run for each date cell and this means there can be multiple requests especially while scrolling 🤔

@rolfsmeds
Copy link
Collaborator Author

Argh, well, yes, of course it does, now that I think about it. And thanks to the design of the picker, it's really easy to scroll a lot very quickly.

Of course multiple "pages" of months could be evaluated each time, a few on each side of the month requested, but even that might be tricky considering how quickly one can scroll in the picker.

I suppose one "solution" could be to initially render all dates as disabled or "undecided", delay the availability evaluation until scrolling has stopped, and then update the dates when you have the results. But I worry that the UX would be awful.

@knoobie
Copy link
Contributor

knoobie commented Mar 29, 2022

Thinking out load: instead of creating a "high level" API on the server.. could this be abstracted to something like

datePicker.disallowSaturday():
datePicker.disallowSunday();
// allows to specify a full date with year, month and day 
// or month and day to allow the client&service side to automatically disable all years with this date
datePicker.disallowSpecificDates("2022-12-24", "12-25");

This does not allow for full flexible solutions like the shown lambda above.. but would drastically reduce the amount of client/server communication.

@jouni
Copy link
Member

jouni commented Mar 29, 2022

Yeah, that might be good enough. Though instead of datePicker.disallowSaturday() (and Sunday), why not datePicker.disallowWeekDays(6, 7)?

@Artur-
Copy link
Member

Artur- commented Mar 29, 2022

Why you should keep min/max
Min/max can affect how the popup behaves so that it stops scrolling when reaching a limit. With an arbitrary "is available" you would need to allow the user to keep scrolling to see if a date 100 years ahead happens to be OK.

The web component API
Why not make that as good and flexible as possible, i.e. a lambda callback. There are no performance problems there

The Java API
Make it "performant enough" by default. Make it possible to use the web component API when you really need all the flexibility. That allows you to optimize the Java API for the most common cases without excluding any use case.

Before optimizing the API though: Which are the primary 3 use cases this is meant for? Disabling weekends? Disabling days when the veterinary has no available timeslots?

@rolfsmeds
Copy link
Collaborator Author

Before optimizing the API though: Which are the primary 3 use cases this is meant for? Disabling weekends? Disabling days when the veterinary has no available timeslots?

Those two are exactly the ones I've had in mind. Don't have a third one 😁

Obviously the first one, disabling weekends or specific weekdays in general, could be solved with a much simpler API and implementation without performance issues, but the second one, disabling, or even distinguishing arbitrary dates is also a fairly common use case.

(Also I just realized that this is very similar to styling individual dates according to some logic as requested in vaadin/web-components#1925)

I guess we could have a Java API that makes it easy to set a clientside lambda callback... and in cases where that callback does its thing solely based on logic, you could implement that logic with JS, but how do you provide it with data for cases where the evaluation is based on things like the availability of appointment time slots?

@Artur-
Copy link
Member

Artur- commented Mar 29, 2022

As a user, I would prefer to have the callback lambda in Java (as described above) and have you figure out how to make the communication efficient and which dates to check :) It is a bit similar to caching rows in a grid instead of fetching one cell at a time

@OlliTietavainenVaadin
Copy link
Member

Maybe I'm pointing out the obvious, but disabling weekdays like Saturdays or Sundays is only covering one use case; you might also want to disable dates like the 4th of April 2022 because the system has scheduled maintenance that day.

@OlliTietavainenVaadin
Copy link
Member

You might also want to disable a range between June 1st to August 1st because that's when your summer holiday is.

@rolfsmeds
Copy link
Collaborator Author

The use case mentioned above, "Disabling days when the veterinary has no available timeslots", is precisely that: any arbitrary dates.

@ghost
Copy link

ghost commented Jan 4, 2023

This request is exactly what we need in our use of the vaadin date picker, except we have no need for the Java side of things. We use the date picker purely as a web component consumed within a LitElement. Would it be acceptable for us to develop a PR that implements a Javascript-only implementation where the is-date-available function is provided as a Javascript function that returns boolean? This is how our legacy datepicker is implemented and we're hoping to replace it with the Vaadin date picker yet keep this critical piece of functionality for disabling arbitrary dates.

@rolfsmeds
Copy link
Collaborator Author

@dethell-jh we will want to have that Java API also. Although we could of course implement that ourselves, the clientside implementation needs to be built in a way that makes it possible to implement the Java API in a sufficiently flexible and performant way.

We can try to come up with a technical design for this, after which you can contribute your clientside implementation.

@TatuLund
Copy link
Contributor

TatuLund commented Jan 5, 2023

I think quite common use case for this feature would be to disable selection of non-working days / i.e. allow selecting only dates that are open for business. This naturally can in practice mean any combination of arbitrary dates, as bank holidays differ by country, and some businesses are open regardless of those, etc. Alternative case could be some sort of availability restriction, i.e. disable selection of dates that are "fully booked".

Currently doing this is quite complex in even simple scenarios. I have an example of just disabling weekends here using combination of CSS and custom validator

https://github.com/TatuLund/hilla-demo/blob/master/frontend/themes/hillacrmtutorial/components/vaadin-month-calendar.css

And

https://github.com/TatuLund/hilla-demo/blob/master/frontend/util/validators.ts

The example is Hilla application, but CSS applies both Hilla/Flow and similar custom validator can be applied Java.

@ghost
Copy link

ghost commented Jan 6, 2023

Just pushed up a draft PR with a working, client-side-only implementation. Figured it was worth seeing an implementation to assist in the discussion. Per the comment there in the PR we as a team are questioning whether any implementer will really want to have server-side, Java-based logic to control the isDateAvailable logic. Seems like a ton of API traffic to paint the calendars.

@knoobie
Copy link
Contributor

knoobie commented Jan 6, 2023

Now you know exactly the hard part about this API, because we Java devs wanna have fun with that feature as well 🤓

@web-padawan
Copy link
Member

Per the comment there in the PR we as a team are questioning whether any implementer will really want to have server-side, Java-based logic to control the isDateAvailable logic. Seems like a ton of API traffic to paint the calendars.

This is why this feature was postponed. We might need to figure out a better approach that would not have this problem.
IMO the set of dates that need to be disallowed for selection could be set separately e.g. via setDisabledDates().

@ghost
Copy link

ghost commented Jan 9, 2023

This is why this feature was postponed. We might need to figure out a better approach that would not have this problem. IMO the set of dates that need to be disallowed for selection could be set separately e.g. via setDisabledDates().

Does the team here think it is acceptable to have some Javascript-only validation for things like validation as a first step and have a feature request to explore the Java-side later? Seems a shame to hold up features in the web components just because there is no corollary in the hilla/Java side of things. I came from the Java framework world (way back in Tapestry and prior) so I definitely get the attraction of having this available to Java developers. Just think there could be some middle ground as we progress the web components forward.

For now we intend to go ahead and get working tests in the PR and publish the PR as a working example of how it can be done and let y'all decide what is best. We'll work from a fork if we need to, though not ideal.

@jouni
Copy link
Member

jouni commented Jan 10, 2023

I think it should be fine to have some features only available in the web component. Just like we have some features that are only available in the Java/Flow component. Not ideal, but can be justified in some cases.

@rolfsmeds
Copy link
Collaborator Author

I'd say it's ok to have a WC-only feature, but it should be built in a way that can accommodate Java-support later.

@jouni
Copy link
Member

jouni commented Jan 10, 2023

What do you mean exactly? Do you mean that it should be possible to use the WC-only feature together with whatever Java/Flow feature is added later? Or that the WC-only feature should be able to handle the Java/Flow feature in the future as well? The latter implies that it's not really only for the web component.

I was in my mind thinking that there could be both APIs, this client-only isDateAvailable callback function, which is called on every single date element, and in the future a more async-friendly setAvailableDates/setDisabledDates method, which Flow can use.

@web-padawan
Copy link
Member

We could indeed have isDateAvailable in the web component and then build Flow component API on top of that.
In case of setDisabledDates it would essentially mean configuring a custom isDateAvailable in the connector.

Some examples of naming from existing design systems:

@ghost
Copy link

ghost commented Jan 10, 2023

This is all good news. I'll get the tests finished off for this PR and change it to Ready to Review.

@rolfsmeds
Copy link
Collaborator Author

@jouni these are exactly the kinds of design decisions that should be made before we bring this to the WC

@ghost
Copy link

ghost commented Jan 17, 2023

I have added unit tests and changed the PR status to Ready to Review. I couldn't figure out how to run a coverage report so I'm assuming I've added enough coverage at this point with these tests.

vaadin/web-components#5252

@stefanuebe
Copy link

I'd like to add the requirement/wish, that it should also be possible to define tooltips for certain disabled dates, so that the user gets the info why the day is disabled, e. g. "Christmas", "New Year", "Day Of The Tentacle", etc.

@ghost
Copy link

ghost commented Oct 17, 2023

@web-padawan or others, can I get some clarification on the AC for the behavior of keyboard usage? If you look at the PR comments we are working on what happens if you try to key over a disabled date. The present behavior with min/max is that it will not let you arrow up below the min or arrow down past the max. With the isDateDisabled functionality we need to allow a user to key over a disabled date because the logic of skipping to the next available date could make for some awkward jumps in focus depending on what dates are disabled. So we opted to allow keying over a disabled date. In doing that, the behavior of min/max also changed where it now allows keyboarding below the min date or above the max.

Is that too much of a breaking change in keyboard behavior or is this an acceptable change given the need to allow keying over a disabled date. Either way, could someone update the AC to reflect the desired behavior of keyboard navigation with this new function?

@rolfsmeds
Copy link
Collaborator Author

Hi @dethell-jh, it would be preferable to retain the old behavior of dates outside the min/max range, so as to avoid a behavioral change in a minor version. Added an acceptance criteria about that.

The rationale for the difference in behavior between min/max and arbitrary disabled dates is that there is that there is no reason for the user to ever go outside the min/max-range as there is nothing selectable there, whereas arbitrary disabled dates occur in between selectable dates, and 1) we don't want to confuse the user by jumping to some other date than the one they were going to, and 2) focusing a disabled date is the only way to convey that it's disabled to screen reader users.

@ghost
Copy link

ghost commented Oct 19, 2023

Hi @dethell-jh, it would be preferable to retain the old behavior of dates outside the min/max range, so as to avoid a behavioral change in a minor version. Added an acceptance criteria about that.

The rationale for the difference in behavior between min/max and arbitrary disabled dates is that there is that there is no reason for the user to ever go outside the min/max-range as there is nothing selectable there, whereas arbitrary disabled dates occur in between selectable dates, and 1) we don't want to confuse the user by jumping to some other date than the one they were going to, and 2) focusing a disabled date is the only way to convey that it's disabled to screen reader users.

@rolfsmeds thanks for clarifying. I'll run with that AC.

@Osiris-Team
Copy link

Osiris-Team commented Apr 15, 2024

Any ETA for this? I would have thought this is a highly demanded feature. Especially for bookings and such.
Also is there any workaround for this right now, like an addon that adds this feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acceptance criteria used for the acceptance criteria checklist DS Design System feature (e.g. component)
Projects
Status: Under consideration
Development

No branches or pull requests

9 participants