Skip to content

feat!: datetimepicker validation improvements #8986

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

Merged
merged 32 commits into from
May 5, 2025

Conversation

ugur-vaadin
Copy link
Contributor

@ugur-vaadin ugur-vaadin commented Apr 19, 2025

Description

  • Adds unparsable-change event for the web component.
  • Handles the case where one of the fields is still empty. Validates if
    • the new value in the edited field is unparsable
    • the edited field is date picker and the date is out of range set using min and/or max
  • improves validation consistency. Except for cases where the the edited field is valid after the edit and the DateTimePicker is still being edited, the validation behaves the same for Enter, outside click, and blur.
  • Adds tests that verify the validation logic and change events using combinations of initial states and field updates.

Based on the prototype implementation.

Part of #6697.

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/contributing/pr
  • 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.
  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

@ugur-vaadin ugur-vaadin requested a review from yuriy-fix April 25, 2025 07:15
@vursen
Copy link
Contributor

vursen commented Apr 29, 2025

Looks like this change and this change together break required validation on blur, which works in other components:

Screen.Recording.2025-04-29.at.14.36.43.mov

@ugur-vaadin
Copy link
Contributor Author

Looks like this change and this change together break required validation on blur, which works in other components:

Screen.Recording.2025-04-29.at.14.36.43.mov

I think that is my misunderstanding rather than an implementation error. My understanding was that the validation should not run when the component is blurred with no change to the input even if it is required.

@vursen
Copy link
Contributor

vursen commented Apr 30, 2025

My understanding was that the validation should not run when the component is blurred with no change to the input even if it is required.

Yeah, there is a difference in component behavior: web components should validate on blur, whereas flow components should not.

@ugur-vaadin
Copy link
Contributor Author

Looks like this change and this change together break required validation on blur, which works in other components:

Screen.Recording.2025-04-29.at.14.36.43.mov

This was fixed and the tests for validating on blur were reverted.

@vursen
Copy link
Contributor

vursen commented May 1, 2025

This was fixed and the tests for validating on blur were reverted.

I guess this place needs to be fixed too, as otherwise required validation doesn't occur when the overlay is closed by clicking outside – a case that is also treated as blur in other components such as date-picker, even though focus is restored afterwards.

@ugur-vaadin
Copy link
Contributor Author

This was fixed and the tests for validating on blur were reverted.

I guess this place needs to be fixed too, as otherwise required validation doesn't occur when the overlay is closed by clicking outside – a case that is also treated as blur in other components such as date-picker, even though focus is restored afterwards.

Fixed and added tests.

vursen
vursen previously approved these changes May 2, 2025
if (isOutsideClick) {
this.__outsideClickInProgress = true;

setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use queueMicrotask here to limit more preciesely how long this flag stays true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that queueMicrotask does not provide enough of a duration for the flag to stay true.

@vursen vursen dismissed their stale review May 2, 2025 06:34

Pressed the wrong button :)

@vursen vursen changed the title feat: datetimepicker validation improvements feat!: datetimepicker validation improvements May 5, 2025
});

it('should commit as unparsable on Enter', async () => {
await enterParsableInput(picker);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this to beforeEach

Suggested change
await enterParsableInput(picker);
await enterParsableInput(picker);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved.

});

it('should commit as unparsable on Enter', async () => {
await enterUnparsableInput(picker);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this to beforeEach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved.

Copy link

sonarqubecloud bot commented May 5, 2025

@ugur-vaadin ugur-vaadin merged commit 0770cde into main May 5, 2025
9 checks passed
@ugur-vaadin ugur-vaadin deleted the feat-datetimepicker-validation-improvements branch May 5, 2025 17:58
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.8.0.rc2 and is also targeting the upcoming stable 24.8.0 version.

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.

3 participants