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

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

Merged
merged 12 commits into from
Oct 13, 2023

Conversation

vursen
Copy link
Contributor

@vursen vursen commented Oct 4, 2023

Description

The PR introduces a new event unparsable-change to date-picker. This event is fired when the user attempts to commit or clear input that the component has failed to parse as a date. This event is intended as a supplement to the change event, which means it will be fired only when there is no change event.

Here is a table that illustrates which event is fired based on the nature of the value change:

Value change Event Note
empty => parsable change
empty => unparsable unparsable-change
parsable => empty change
parsable => parsable change
parsable => unparsable change The value property changes to an empty string
unparsable => empty unparsable-change
unparsable => parsable change The value property changes to a non-empty string
unparsable => unparsable unparsable-change

The PR also prevents date-picker from validating on Enter if the input hasn't changed (related to #6591).

Part of vaadin/flow-components#5537

Type of change

  • Feature

} else if (this.__committedUnparsableValue !== this._unparsableValue) {
result = true;
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

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

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

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

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.

@vursen vursen force-pushed the feat/date-picker/unparsable-change-event branch from 0666023 to 60ea348 Compare October 4, 2023 16:51
@vursen vursen force-pushed the feat/date-picker/unparsable-change-event branch 2 times, most recently from 6e4f5cd to 9cf2d1c Compare October 6, 2023 05:47
@@ -911,7 +957,6 @@ export const DatePickerMixin = (subclass) =>
} else {
this.__keepInputValue = true;
this.__commitDate(null);
this._selectedDate = null;
Copy link
Contributor Author

@vursen vursen Oct 6, 2023

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).

@vursen
Copy link
Contributor Author

vursen commented Oct 6, 2023

note: The validation can now be triggered twice on blur or outside click when it coincides with change. I considered that it's something that can be optimized later since (1) the Flow components will no longer rely on client-side validation timing (2) validate() calls themselves are pretty cheap in web-components.

@vursen vursen marked this pull request as ready for review October 9, 2023 13:43
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@vursen vursen merged commit 692f383 into main Oct 13, 2023
9 checks passed
@vursen vursen deleted the feat/date-picker/unparsable-change-event branch October 13, 2023 07:14
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants