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.

Sign up for GitHub

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

feat(date-picker): add unparsable-change event #6594

Merged
merged 12 commits into from
Oct 13, 2023
Merged

feat(date-picker): add unparsable-change event #6594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a4d588e
fix: do not validate on Enter if no value commit occurred
vursen Oct 3, 2023
3062576
fire unparsable-change event
vursen Oct 4, 2023
3731fa1
update tests
vursen Oct 4, 2023
4606b0c
fix the code comment
vursen Oct 4, 2023
8be6ae4
fix code comment
vursen Oct 4, 2023
4bfa618
make __unparsableValue private
vursen Oct 4, 2023
a2f522f
simplify the solution
vursen Oct 5, 2023
4defa9d
revert code comment
vursen Oct 6, 2023
375df5a
improve JSDoc
vursen Oct 9, 2023
67a74e5
add more unit tests
vursen Oct 11, 2023
3b84d9e
store unparsableValue in a variable
vursen Oct 12, 2023
5eade8c
Update packages/date-picker/src/vaadin-date-picker-mixin.js
vursen Oct 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 59 additions & 21 deletions packages/date-picker/src/vaadin-date-picker-mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,20 @@ export const DatePickerMixin = (subclass) =>
return null;
}

/**
* The input element's value when it cannot be parsed as a date, and an empty string otherwise.
*
* @return {string}
* @private
*/
get __unparsableValue() {
if (!this._inputElementValue || this.__parseDate(this._inputElementValue)) {
return '';
}

return this._inputElementValue;
}

/**
* Override an event listener from `DelegateFocusMixin`
* @protected
Expand Down Expand Up @@ -645,27 +659,52 @@ export const DatePickerMixin = (subclass) =>
this._shouldKeepFocusRing = focused && this._keyboardActive;
}

/** @private */
__dispatchChange() {
this.validate();
this.dispatchEvent(new CustomEvent('change', { bubbles: true }));
/**
* Depending on the nature of the value change that has occurred since
* the last commit attempt, triggers validation and fires an event:
*
* Value change | Event
* :------------------------|:------------------
* empty => parsable | change
* empty => unparsable | unparsable-change
* parsable => empty | change
* parsable => parsable | change
* parsable => unparsable | change
* unparsable => empty | unparsable-change
* unparsable => parsable | change
* unparsable => unparsable | unparsable-change
*
* @private
*/
__commitValueChange() {
const unparsableValue = this.__unparsableValue;

if (this.__committedValue !== this.value) {
this.validate();
this.dispatchEvent(new CustomEvent('change', { bubbles: true }));
} else if (this.__committedUnparsableValue !== unparsableValue) {
this.validate();
this.dispatchEvent(new CustomEvent('unparsable-change'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to pass the unparseable value to event to allow handling with flow? This would reduce the need for additionals calls to __committedUnparsableValue or syncing it with the server for better (?) performance.

Copy link
Contributor Author

@vursen vursen Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we've considered this option too, but in that case we will need to pass it also with the change event. Note, the component fires change rather than unparsable-change when the user changes parsable input to unparsable and vice versa. So, ultimately, we come to the same outcome as with property synchronization in Flow. Both approaches seem to offer relatively similar performance: all the data is sent within a single round-trip – which is triggered by change or unparsable-change. Property synchronization appears to be more preferable, as it doesn't require changing the API of the change event.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understandable, but wouldn't that be a good change anyway? This would align the change event also to the change event from the normal inputs where you can access the value https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/change_event

Copy link
Contributor Author

@vursen vursen Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would align the change event also to the change event from the normal inputs where you can access the value

Just to make sure we understand each other, did you mean passing the input value via event.detail or some other way? I'm not sure if the native change event does it this way. The native event provides access to the target element through which you can access the value, yes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, dismiss my comment 😬 it's done by accessing the event.target.value instead of details

}

this.__committedValue = this.value;
this.__committedUnparsableValue = unparsableValue;
}

/**
* Sets the given date as the value and fires a change event
* if the value has changed.
* Sets the given date as the value and commits it.
*
* @param {Date} date
* @private
*/
__commitDate(date) {
const prevValue = this.value;

// Prevent the value observer from treating the following value change
// as initiated programmatically by the developer, and therefore
// from automatically committing it without a change event.
this.__keepCommittedValue = true;
this._selectedDate = date;

if (prevValue !== this.value) {
this.__dispatchChange();
}
this.__keepCommittedValue = false;
DiegoCardoso marked this conversation as resolved.
Show resolved Hide resolved
this.__commitValueChange();
}

/** @private */
Expand Down Expand Up @@ -801,6 +840,11 @@ export const DatePickerMixin = (subclass) =>
this._selectedDate = null;
}

if (!this.__keepCommittedValue) {
this.__committedValue = this.value;
this.__committedUnparsableValue = '';
}

this._toggleHasValue(this._hasValue);
}

Expand Down Expand Up @@ -892,9 +936,9 @@ export const DatePickerMixin = (subclass) =>
}

/**
* Tries to parse the input element's value as a date. When succeeds,
* sets the resulting date as the value and fires a change event
* (if the value has changed). If no i18n parser is provided, sets
* Tries to parse the input element's value as a date. If the input value
* is parsable, commits the resulting date as the value. Otherwise, commits
* an empty string as the value. If no i18n parser is provided, commits
* the focused date as the value.
*
* @private
Expand All @@ -911,7 +955,6 @@ export const DatePickerMixin = (subclass) =>
} else {
this.__keepInputValue = true;
this.__commitDate(null);
this._selectedDate = null;
Copy link
Contributor Author

@vursen vursen Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: This line is unnecessary since the selected date is already updated by __commitDate(null).

this.__keepInputValue = false;
}
} else if (this._focusedDate) {
Expand All @@ -927,7 +970,6 @@ export const DatePickerMixin = (subclass) =>
this.__showOthers();
this.__showOthers = null;
}

window.removeEventListener('scroll', this._boundOnScroll, true);

this.__commitParsedOrFocusedDate();
Expand Down Expand Up @@ -1080,16 +1122,12 @@ export const DatePickerMixin = (subclass) =>
* @override
*/
_onEnter(_event) {
const oldValue = this.value;
if (this.opened) {
// Closing will implicitly select parsed or focused date
this.close();
} else {
this.__commitParsedOrFocusedDate();
}
if (oldValue === this.value) {
Copy link
Contributor Author

@vursen vursen Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: Now that the validation is the responsibility of the commit logic, we no longer need to trigger validation here, which fixes #6591.

this.validate();
}
}

/**
Expand Down
24 changes: 24 additions & 0 deletions packages/date-picker/src/vaadin-date-picker.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ export type DatePickerChangeEvent = Event & {
target: DatePicker;
};

/**
* Fired when the user commits an unparsable value change and there is no change event.
*/
export type DatePickerUnparsableChangeEvent = CustomEvent;

/**
* Fired when the `opened` property changes.
*/
Expand Down Expand Up @@ -43,6 +48,8 @@ export interface DatePickerCustomEventMap {

'value-changed': DatePickerValueChangedEvent;

'unparsable-change': DatePickerUnparsableChangeEvent;

validated: DatePickerValidatedEvent;
}

Expand Down Expand Up @@ -148,7 +155,24 @@ export interface DatePickerEventMap extends HTMLElementEventMap, DatePickerCusto
*
* See [Styling Components](https://vaadin.com/docs/latest/styling/styling-components) documentation.
*
* ### Change events
*
* Depending on the nature of the value change that the user attempts to commit e.g. by pressing Enter,
* the component can fire either a `change` event or an `unparsable-change` event:
*
* Value change | Event
* :------------------------|:------------------
* empty => parsable | change
* empty => unparsable | unparsable-change
* parsable => empty | change
* parsable => parsable | change
* parsable => unparsable | change
* unparsable => empty | unparsable-change
* unparsable => parsable | change
* unparsable => unparsable | unparsable-change
*
* @fires {Event} change - Fired when the user commits a value change.
* @fires {Event} unparsable-change Fired when the user commits an unparsable value change and there is no change event.
* @fires {CustomEvent} invalid-changed - Fired when the `invalid` property changes.
* @fires {CustomEvent} opened-changed - Fired when the `opened` property changes.
* @fires {CustomEvent} value-changed - Fired when the `value` property changes.
Expand Down
17 changes: 17 additions & 0 deletions packages/date-picker/src/vaadin-date-picker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,24 @@ registerStyles('vaadin-date-picker', [inputFieldShared, datePickerStyles], { mod
*
* See [Styling Components](https://vaadin.com/docs/latest/styling/styling-components) documentation.
*
* ### Change events
*
* Depending on the nature of the value change that the user attempts to commit e.g. by pressing Enter,
* the component can fire either a `change` event or an `unparsable-change` event:
*
* Value change | Event
* :------------------------|:------------------
* empty => parsable | change
* empty => unparsable | unparsable-change
* parsable => empty | change
* parsable => parsable | change
* parsable => unparsable | change
* unparsable => empty | unparsable-change
* unparsable => parsable | change
* unparsable => unparsable | unparsable-change
*
* @fires {Event} change - Fired when the user commits a value change.
* @fires {Event} unparsable-change Fired when the user commits an unparsable value change and there is no change event.
* @fires {CustomEvent} invalid-changed - Fired when the `invalid` property changes.
* @fires {CustomEvent} opened-changed - Fired when the `opened` property changes.
* @fires {CustomEvent} value-changed - Fired when the `value` property changes.
Expand Down
84 changes: 66 additions & 18 deletions packages/date-picker/test/value-commit-auto-open-disabled.common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import sinon from 'sinon';
const TODAY_DATE = new Date().toISOString().split('T')[0];

describe('value commit - autoOpenDisabled', () => {
let datePicker, valueChangedSpy, validateSpy, changeSpy;
let datePicker, valueChangedSpy, validateSpy, changeSpy, unparsableChangeSpy;

function expectNoValueCommit() {
expect(valueChangedSpy).to.be.not.called;
Expand All @@ -19,14 +19,25 @@ describe('value commit - autoOpenDisabled', () => {
// TODO: Optimize the number of validation runs.
expect(validateSpy).to.be.called;
expect(validateSpy.firstCall).to.be.calledAfter(valueChangedSpy.firstCall);
expect(unparsableChangeSpy).to.be.not.called;
expect(changeSpy).to.be.calledOnce;
expect(changeSpy.firstCall).to.be.calledAfter(validateSpy.firstCall);
expect(datePicker.value).to.equal(value);
}

function expectUnparsableValueCommit() {
expect(valueChangedSpy).to.be.not.called;
// TODO: Optimize the number of validation runs.
expect(validateSpy).to.be.called;
expect(changeSpy).to.be.not.called;
expect(unparsableChangeSpy).to.be.calledOnce;
expect(unparsableChangeSpy).to.be.calledAfter(validateSpy);
}

function expectValidationOnly() {
expect(valueChangedSpy).to.be.not.called;
expect(validateSpy).to.be.calledOnce;
// TODO: Optimize the number of validation runs.
expect(validateSpy).to.be.called;
expect(changeSpy).to.be.not.called;
}

Expand All @@ -41,6 +52,9 @@ describe('value commit - autoOpenDisabled', () => {
changeSpy = sinon.spy().named('changeSpy');
datePicker.addEventListener('change', changeSpy);

unparsableChangeSpy = sinon.spy().named('unparsableChangeSpy');
datePicker.addEventListener('unparsable-change', unparsableChangeSpy);

datePicker.focus();
});

Expand All @@ -50,9 +64,9 @@ describe('value commit - autoOpenDisabled', () => {
expectValidationOnly();
});

it('should not commit but validate on Enter', async () => {
it('should not commit on Enter', async () => {
await sendKeys({ press: 'Enter' });
expectValidationOnly();
expectNoValueCommit();
});

it('should not commit but validate on outside click', () => {
Expand Down Expand Up @@ -130,21 +144,21 @@ describe('value commit - autoOpenDisabled', () => {
await sendKeys({ type: 'foo' });
});

it('should not commit but validate on blur', () => {
it('should commit as unparsable value change on blur', () => {
datePicker.blur();
expectValidationOnly();
expectUnparsableValueCommit();
expect(datePicker.inputElement.value).to.equal('foo');
});

it('should not commit but validate on Enter', async () => {
it('should commit as unparsable value change on Enter', async () => {
await sendKeys({ press: 'Enter' });
expectValidationOnly();
expectUnparsableValueCommit();
expect(datePicker.inputElement.value).to.equal('foo');
});

it('should not commit but validate on outside click', () => {
it('should commit as unparsable value change on outside click', () => {
outsideClick();
expectValidationOnly();
expectUnparsableValueCommit();
expect(datePicker.inputElement.value).to.equal('foo');
});

Expand All @@ -160,6 +174,7 @@ describe('value commit - autoOpenDisabled', () => {
await sendKeys({ type: 'foo' });
await sendKeys({ press: 'Enter' });
validateSpy.resetHistory();
unparsableChangeSpy.resetHistory();
});

describe('input cleared with Backspace', () => {
Expand All @@ -168,19 +183,46 @@ describe('value commit - autoOpenDisabled', () => {
await sendKeys({ press: 'Backspace' });
});

it('should not commit but validate on blur', () => {
it('should commit as unparsable value change on blur', () => {
datePicker.blur();
expectValidationOnly();
expectUnparsableValueCommit();
});

it('should not commit but validate on Enter', async () => {
it('should commit as unparsable value change on Enter', async () => {
await sendKeys({ press: 'Enter' });
expectValidationOnly();
expectUnparsableValueCommit();
});

it('should not commit but validate on outside click', () => {
it('should commit as unparsable value change on outside click', () => {
outsideClick();
expectValidationOnly();
expectUnparsableValueCommit();
});

it('should clear and commit as unparsable value change on Escape', async () => {
await sendKeys({ press: 'Escape' });
expectUnparsableValueCommit();
expect(datePicker.inputElement.value).to.equal('');
});
});

describe('unparsable input changed', () => {
beforeEach(async () => {
await sendKeys({ type: 'bar' });
});

it('should commit as unparsable value change on blur', () => {
datePicker.blur();
expectUnparsableValueCommit();
});

it('should commit as unparsable value change on Enter', async () => {
await sendKeys({ press: 'Enter' });
expectUnparsableValueCommit();
});

it('should commit as unparsable value change on outside click', () => {
outsideClick();
expectUnparsableValueCommit();
});
});
});
Expand All @@ -201,9 +243,9 @@ describe('value commit - autoOpenDisabled', () => {
expectValidationOnly();
});

it('should not commit but validate on Enter', async () => {
it('should not commit on Enter', async () => {
await sendKeys({ press: 'Enter' });
expectValidationOnly();
expectNoValueCommit();
});

it('should not commit on Escape', async () => {
Expand Down Expand Up @@ -246,6 +288,12 @@ describe('value commit - autoOpenDisabled', () => {
await sendKeys({ type: 'foo' });
});

it('should commit an empty value on blur', () => {
datePicker.blur();
expectValueCommit('');
expect(datePicker.inputElement.value).to.equal('foo');
});

it('should commit an empty value on Enter', async () => {
await sendKeys({ press: 'Enter' });
expectValueCommit('');
Expand Down
Loading