feat: Allow disabling arbitrary dates in vaadin-date-picker via isDateAvailable property #5252

ghost · 2023-01-06T19:20:55Z

Description

This change updates the vaadin-date-picker to allow disabling arbitrary dates via a new isDateAvailable 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

Bugfix
Feature

Checklist

I have read the contribution guide: https://vaadin.com/docs/latest/contributing/overview
I have added a description following the guideline.
The issue is created in the corresponding repository and I have referenced it.
I have added tests to ensure my change is effective and works as intended.
New and existing tests are passing locally with my change.
I have performed self-review and corrected misspellings.

Additional for `Feature` type of change

Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

ghost · 2023-01-06T19:24:08Z

I went ahead and pushed up a draft of my changes even though I haven't added the tests yet. I wanted a starting point to discuss issue #1820, specifically the comments in the related platform issue where the discussion includes whether we change the Java API.

Our team doesn't use the Java/hilla code at all, but we understand the general use case for validators that are hosted on the API side. However, for this specific use-case, we all questioned why anyone would really want a server-side isDateAvailable function because it is going to get hit hundreds of times in rapid succession as it pre-renders months worth of month data in preparation for scrolling. Is is really realistic to expect we want the performance hit of Java-side logic for this specific case?

sonarcloud · 2023-01-17T22:12:18Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sissbruecker

Sorry for the late review.

The overall approach of introducing a customizable function makes sense to me, and should be a sufficient basis for introducing the same feature in the Flow API. Some API could be improved, and there are some missing tests.

sissbruecker · 2023-05-02T08:36:21Z

packages/date-picker/src/vaadin-date-picker-mixin.d.ts

@@ -235,6 +235,11 @@ export declare class DatePickerMixinClass {
   */
  max: string | undefined;

+  /**
+   * Function to override the default function for determining if a date is available.


Let's use the same description as in the JSDoc here.

sissbruecker · 2023-05-02T08:48:20Z

packages/date-picker/src/vaadin-date-picker-mixin.js

+        /**
+         * A function that is used to determine if a date should be disabled.
+         * 
+         * @type {function(Date): boolean | undefined}
+         */
+        isDateAvailable: {
+          type: Function,
+        },


Since the main purpose of the new feature is to disable specific dates, I think it would make more sense to name this to isDateDisabled.

While passing a date instance is reasonable, I think for consistency we should align this with other functions that can be configured currently: formatDate and parseDate via the i18n property. Those take and return an object of the form: { year: number, month: number, day: number }.

The param should also be documented in some detail in the JSDoc.

Commenting here even though I'm moving to a new PR. Are you sure we should make the object sent to the isDateDisabled function the DatePickerDate interface? The reason I ask is the main place this gets called is in vaadin-date-picker-helper in the dateAllowed function. That function receives a Javascript Date rather than a vaadin DatePickerDate shape.

sissbruecker · 2023-05-02T08:55:34Z

packages/date-picker/src/vaadin-date-picker-helper.js

-export function dateAllowed(date, min, max) {
-  return (!min || date >= min) && (!max || date <= max);
+export function dateAllowed(date, min, max, isDateAvailable) {
+  const dateIsAvailable = isDateAvailable && typeof isDateAvailable === 'function' ? isDateAvailable(date) : true;


Suggested change

const dateIsAvailable = isDateAvailable && typeof isDateAvailable === 'function' ? isDateAvailable(date) : true;

const dateIsAvailable = typeof isDateAvailable === 'function' ? isDateAvailable(date) : true;

The typeof check should be enough.

sissbruecker · 2023-05-02T08:57:18Z

packages/date-picker/src/vaadin-date-picker-helper.js

@@ -61,8 +61,9 @@ export function dateEquals(date1, date2) {
 * @param {Date} max Range end
 * @return {boolean} True if the date is in the range
 */
-export function dateAllowed(date, min, max) {
-  return (!min || date >= min) && (!max || date <= max);
+export function dateAllowed(date, min, max, isDateAvailable) {


Let's add the new parameter to the JSDoc.

sissbruecker · 2023-05-02T09:00:56Z

packages/date-picker/src/vaadin-date-picker-mixin.js

      const minMaxValid = !this._selectedDate || dateAllowed(this._selectedDate, this._minDate, this._maxDate);
+      const isDateAvailableValid = this.isDateAvailable ? this.isDateAvailable(this._selectedDate) : true;


Since dateAllowed is capable of checking for whether the date should be disabled, it seems unnecessary to call isDateAvailable separately. Instead, I would suggest to pass the function into dateAllowed, and generalize the constant name from minMaxValid to something more general like isDateValid.

Much cleaner.

sissbruecker · 2023-05-02T09:02:14Z

packages/date-picker/src/vaadin-date-picker-overlay-content.js

-  _dateAllowed(date, min = this.minDate, max = this.maxDate) {
-    return (!min || date >= min) && (!max || date <= max);
+  _dateAllowed(date, min = this.minDate, max = this.maxDate, isDateAvailable = this.isDateAvailable) {
+    return (!min || date >= min) && (!max || date <= max) && (!isDateAvailable || isDateAvailable(date))


I would suggest to reuse the typeof check here, as it's more precise than checking for a falsy value.

sissbruecker · 2023-05-02T09:05:38Z

packages/date-picker/src/vaadin-month-calendar.js

  }

-  __getDayAriaDisabled(date, min, max) {
+  __getDayAriaDisabled(date, min, max, isDateAvailable) {


Doesn't look like the new function is properly passed to this method.

Also indicates a missing test, it looks like month calendar attributes like part and aria-disabled are currently covered by snapshot tests, so a new snapshot test should be added there.

sissbruecker · 2023-05-02T09:26:25Z

packages/date-picker/test/validation.test.js

@@ -214,6 +214,24 @@ describe('validation', () => {
      datePicker._overlayContent._selectDate(new Date('2017-01-01')); // Invalid
    });

+    it('should reflect correct invalid value on value-changed eventListener when using isDateAvailable', (done) => {


There are some additional tests for the max attribute, respective tests for the new function should be added:

web-components/packages/date-picker/test/validation.test.js

Lines 61 to 67 in 877d1d2

it('should validate when the field has max', async () => {

datePicker.max = '2020-01-01';

document.body.appendChild(datePicker);

await nextRender();

expect(validateSpy.calledOnce).to.be.true;

});

web-components/packages/date-picker/test/validation.test.js

Lines 132 to 142 in 877d1d2

it('should not validate on max change when no value is provided', () => {

datePicker.max = '2020-01-01';

expect(validateSpy.called).to.be.false;

});

it('should validate on max change when a value is provided', () => {

datePicker.value = '2020-01-01';

validateSpy.resetHistory();

datePicker.max = '2020-01-01';

expect(validateSpy.calledOnce).to.be.true;

});

ghost · 2023-05-19T09:45:55Z

The company I work for has pivoted away from using the vaadin-date-picker so we don't have cycles to put to this PR. I'm closing it and if anyone wants to pick up the work they are free to take the work done here and update it per the review comments.

ghost · 2023-09-11T18:14:59Z

Re-opening. We have pivoted back to this web component so we'll polish up the PR and get the remaining tests working so we can have this functionality working.

Co-authored-by: A.J. Roberts <arobertsbnr@users.noreply.github.com>

sonarcloud · 2023-09-20T15:55:02Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

ghost · 2023-09-21T15:19:16Z

I'll create a new PR. Needed to rebase and the renaming of master to main in vaadin meant I needed to follow suit in my PR to get the tests to run correctly.

ghost marked this pull request as ready for review January 17, 2023 22:12

ghost mentioned this pull request Jan 17, 2023

DatePicker support for disabling arbitrary dates vaadin/platform#2867

Open

11 tasks

sissbruecker requested changes May 2, 2023

View reviewed changes

ghost closed this May 19, 2023

ghost reopened this Sep 11, 2023

dethell and others added 4 commits September 14, 2023 16:03

Add isDateAvailable property

2f732de

Further changes to allow disabling arbitrary dates

c90717f

Co-authored-by: A.J. Roberts <arobertsbnr@users.noreply.github.com>

Starting to add tests for isDateAvailable changes

119e21c

Add validation and disabled tests

b1e9d3b

ghost force-pushed the master branch from 877d1d2 to b1e9d3b Compare September 14, 2023 20:13

Merge branch 'vaadin:main' into master

6a2c028

ghost closed this Sep 20, 2023

ghost deleted the master branch September 20, 2023 16:51

ghost mentioned this pull request Oct 5, 2023

feat(date-picker): Add isDateDisabled functionality #6603

Closed

10 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow disabling arbitrary dates in vaadin-date-picker via isDateAvailable property #5252

feat: Allow disabling arbitrary dates in vaadin-date-picker via isDateAvailable property #5252

ghost commented Jan 6, 2023 •

edited by ghost

Loading

ghost commented Jan 6, 2023

sonarcloud bot commented Jan 17, 2023

sissbruecker left a comment

sissbruecker May 2, 2023

sissbruecker May 2, 2023

ghost Sep 21, 2023

sissbruecker May 2, 2023

sissbruecker May 2, 2023

sissbruecker May 2, 2023

ghost Sep 21, 2023

sissbruecker May 2, 2023

sissbruecker May 2, 2023

sissbruecker May 2, 2023 •

edited

Loading

ghost commented May 19, 2023

ghost commented Sep 11, 2023

sonarcloud bot commented Sep 20, 2023

ghost commented Sep 21, 2023

	const dateIsAvailable = isDateAvailable && typeof isDateAvailable === 'function' ? isDateAvailable(date) : true;
	const dateIsAvailable = typeof isDateAvailable === 'function' ? isDateAvailable(date) : true;

		const minMaxValid = !this._selectedDate \|\| dateAllowed(this._selectedDate, this._minDate, this._maxDate);
		const isDateAvailableValid = this.isDateAvailable ? this.isDateAvailable(this._selectedDate) : true;


	it('should validate when the field has max', async () => {
	datePicker.max = '2020-01-01';
	document.body.appendChild(datePicker);
	await nextRender();
	expect(validateSpy.calledOnce).to.be.true;
	});

	it('should not validate on max change when no value is provided', () => {
	datePicker.max = '2020-01-01';
	expect(validateSpy.called).to.be.false;
	});

	it('should validate on max change when a value is provided', () => {
	datePicker.value = '2020-01-01';
	validateSpy.resetHistory();
	datePicker.max = '2020-01-01';
	expect(validateSpy.calledOnce).to.be.true;
	});

feat: Allow disabling arbitrary dates in vaadin-date-picker via isDateAvailable property #5252

feat: Allow disabling arbitrary dates in vaadin-date-picker via isDateAvailable property #5252

Conversation

ghost commented Jan 6, 2023 • edited by ghost Loading

Description

Type of change

Checklist

Additional for Feature type of change

ghost commented Jan 6, 2023

sonarcloud bot commented Jan 17, 2023

sissbruecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sissbruecker May 2, 2023 • edited Loading

Choose a reason for hiding this comment

ghost commented May 19, 2023

ghost commented Sep 11, 2023

sonarcloud bot commented Sep 20, 2023

ghost commented Sep 21, 2023

ghost commented Jan 6, 2023 •

edited by ghost

Loading

Additional for `Feature` type of change

sissbruecker May 2, 2023 •

edited

Loading