feat(date-picker): Add isDateDisabled functionality #6603
Conversation
Co-authored-by: A.J. Roberts <[email protected]>
…nction returns a true boolean
…ion for checking if entire month is disabled
|
@sissbruecker I have recreated this PR per the comments you made on the closed one. The only deviation from your suggestion is noted in the discussion in the closed PR about passing a |
The closed PR is: #5252 |
ghost
changed the title
dev/date-picker.html
Outdated
| <script type="module"> | ||
| import '@vaadin/date-picker'; | ||
| import '@vaadin/tooltip'; | ||
| const isDateDisabled = (date) => !!(date?.getDay() === 0); |
There was a problem hiding this comment.
This is a sample implementation in the demo app. We can drop this or add some docs as needed to describe what's going on.
| */ | ||
| export function dateAllowed(date, min, max) { | ||
| return (!min || date >= min) && (!max || date <= max); | ||
| export function dateAllowed(date, min, max, isDateDisabled) { |
There was a problem hiding this comment.
This is the heavy lifting function that does all the work of deciding a date needs to be disabled. So all the components that need to disable a date end up feeding here.
packages/date-picker/src/vaadin-date-picker-overlay-content-mixin.js
Outdated
Show resolved
Hide resolved
…te type instead of Date
| * @protected | ||
| */ | ||
| _selectDate(dateToSelect) { | ||
| if (!this._dateAllowed(dateToSelect)) { |
There was a problem hiding this comment.
Might need to discuss this change. I updated this to be a boolean function so that the keyboard Enter behavior could be prevented for disabled dates. That allows us to hover on disabled dates without allowing selection of those dates.
| _focusAllowedDate(dateToFocus, diff, keepMonth) { | ||
| if (this._dateAllowed(dateToFocus)) { | ||
| // For this check we do consider the isDateDisabled function because disabled dates are allowed to be focused, just not outside min/max | ||
| if (this._dateAllowed(dateToFocus, undefined, undefined, () => false)) { |
There was a problem hiding this comment.
In this one place we want to bypass any custom isDateDisabled function since we are allowed to focus the date, just not select it.
| }); | ||
|
|
||
| expect(invalidChangedSpy.calledOnce).to.be.true; | ||
| const selectResult = datePicker._overlayContent._selectDate(new Date('2017-01-01')); // Invalid |
There was a problem hiding this comment.
Dropped the check for the invalid change since the new logic will prevent actually selecting an invalid date. Checking the boolean result of _selectDate should be sufficient. Same for below tests.
| const isDateDisabled = (date) => { | ||
| // Exclude weekends and the 16th day of each month: | ||
| const checkDate = new Date(0, 0); | ||
| checkDate.setFullYear(date.year); | ||
| checkDate.setMonth(date.month); | ||
| checkDate.setDate(date.day); | ||
| return checkDate.getDay() === 0 || checkDate.getDay() === 6 || checkDate.getDate() === 16; | ||
| } |
There was a problem hiding this comment.
We recently had a brief internal discussion regarding the parameter format for the
isDateDisabled function, considering the diverse range of use cases. Two potential options:
Option 1: DatePickerDate ({year: 2023, month: 10, day: 10}):
- More structured and explicit, allowing users to access individual components of the date easily.
- Might be easier for disabling specific weekdays or days of months
Option 2: ISO Formatted Date String ("2023-11-10"):
- Familiar format, commonly used in various contexts.
- Same format as the component's
value - Might be easier for disabling specific dates (stored in DB as local date for example)
If a decision between these two options proves challenging, we could consider offering flexibility by providing both ISO formatted date strings and date objects with individual components. For example, an extended DatePickerDate could include both formats as follows:
{
year: 2023,
month: 10,
day: 10,
date: "2023-11-10"
}
There was a problem hiding this comment.
Would the ISO date be optional or should we expect that the DatePickerDate structure always contains all four fields?
There was a problem hiding this comment.
Modifying DatePickerDate might be breaking, so better introduce a new type or extend it (if this is the option we want to go with).
There was a problem hiding this comment.
For now it seems to be working well enough with DatePickerDate. If a developer needs more detailed logic it's easy enough to convert it to a Javascript Date object.
There was a problem hiding this comment.
@rolfsmeds and @yuriy-fix , can you weigh in on this discussion and see if we should alter the DatePickerDate shape per @tomivirkki 's comments or can we proceed with merging this PR as it stands? All other concerns have been addressed save this one.
packages/date-picker/src/vaadin-date-picker-overlay-content-mixin.js
Outdated
Show resolved
Hide resolved
ghost
requested a review
from
|
Kudos, SonarCloud Quality Gate passed!
|
|
@tomivirkki Is there anything that I need to do regarding the Visual tests failing to run due to Saucelabs proxy connect issues? I didn't see a way to retry those tests. |
Nope, we'll take care of this before merging |
|
We will proceed with the PR after branching out for new minor. (~ couple of weeks) |
Any word on this? Wondering if this will be in |
|
Closed. Superseded by #7075 |
ghost
closed this
|
Sorry for all the "ghost" comments. I merged my GitHub accounts. All the commits moved, but GitHub doesn't re-attach discussions/comments/etc. |
3 participants
Description
This change updates the vaadin-date-picker to allow disabling arbitrary dates via a new isDateDisabled property. This property is a function
Fixes #1820 but see this platform issue for specific discussion: vaadin/platform#2867
This PR only addresses the UX-frontend portion. This PR does not attempt to address the Java API side.
Type of change
Checklist
Additional for
Featuretype of change