Add `@validateOn="input", rename "blur" to "focusout" #31

simonihmig · 2023-02-06T17:52:33Z

When playing with validations in a real demo rather than just looking for tests to become green, I realised that the behaviour we likely want for revalidation is not given when using @revalidationOn="change". We want to have revalidation happen as soon as you type (into an invalid field) and it turns valid, without having to focus out of the input (which would effectively be @revalidationOn="blur"). But change does not trigger right while typing for inputs (normal texts inputs, checkboxes, radios and select boxes this is different though), only when focussing out ("committing"). (see change event).

So this introduces another option @revalidationOn="input", listening for the input event:

The PR also takes this as a opportunity to rename @(re)validationOn="blur" to @(re)validationOn="focusout", as this is the event we are actually listening to, and having one value for the public API and another for the internal implementation did not really serve a meaningful purpose (other than maybe blur being a bit more common), but could likely cause confusion.

changeset-bot · 2023-02-06T17:52:36Z

⚠️ No Changeset found

Latest commit: b099152

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

nicolechung · 2023-02-06T18:00:11Z

Release questions:

Are we doing releases yet? Because I guess this would be a breaking change (since we are changing "blur" to "focusout" but since we haven't released anything yet I guess this is not.
If we are doing releases, do we look for the changeset in this PR? (I feel like I have asked this question before in another repo...clearly I don't have a lot of hands on practise with changesets yet)

NullVoxPopuli · 2023-02-06T18:01:57Z

Are we doing releases yet?

not yet -- I've opened this: #30
and it starts us out with beta releases, so breaking changes are non-major.

#30 has all the context for how to start releases if we want to publish to npm

do we look for the changeset in this PR?

as we are all maintainers, yes (again, if we're releasing).
if this PR were from an outside contributor, one of us maintainers would add the changeset before merging.

However, #30 is a blocker for doing any changeset / release stuff

nicolechung · 2023-02-06T18:08:42Z

test-app/tests/helpers/dom.ts

+ * This mimics the behavior of a user typing data into an input without yet focusing out of it. Browsers will only trigger a `change` event when focusing
+ * out of the element, not while typing!
+ *
+ * `fillIn` will basically simulate entering the data *and* kinda focusing out (as it triggers `change`, however not `focusout`, which is impossible to achieve as a real user),


😮 I did NOT know this!

Yeah, had to confirm this for myself by looking up their source.

I should probably create an issue. I guess we cannot change the existing behaviour of fillIn, as that would break every app. But maybe we can upstream some additional helpers that cover the different real-world use cases better...

Filed this: emberjs/ember-test-helpers#1336

NullVoxPopuli · 2023-02-06T18:10:01Z

test-app/tests/helpers/dom.ts

+import { find, triggerEvent } from '@ember/test-helpers';
+
+/**
+ * Fill the provided text into the `value` property of the selected form element, similar to `fillIn`, but *without* implicitly triggering a `change` event.


10/10 would read again

nicolechung · 2023-02-06T18:10:42Z

test-app/tests/integration/components/headless-form-validation-test.gts

+        assert
+          .dom('[data-test-first-name-errors]')
+          .doesNotExist(
+            'validation errors are not rendered before validation happens on change'


Suggestion: on change -> on input. Since this test is now testing for input not change.

Oh ya, thanks! A lot of boilerplate in those tests, that I just copy-pased from the change tests. Fixed that!

test-app/tests/integration/components/headless-form-validation-test.gts

nicolechung · 2023-02-06T18:11:19Z

test-app/tests/integration/components/headless-form-validation-test.gts

+          .dom('[data-test-first-name-errors]')
+          .exists(
+            { count: 1 },
+            'validation errors appear on focusout when validation fails'


Suggestion: on focusout -> on input.

Don't we want to test that there are no errors on the last name field (by accident)?

We do, just a few lines below!

nicolechung · 2023-02-06T18:14:37Z

test-app/tests/integration/components/headless-form-validation-test.gts

+            'validation errors are not rendered before initial validation happens before form is filled in'
+          );
+
+        await fillIn('[data-test-first-name]', 'Foo');


Do we want input here or fillIn?

I think @simonihmig is doing something similar here as the above tests - ensuring fillIn doesn't cause errors to render, but below on line 1976 where he's using input it does

I guess it doesn't matter in this case too because the revalidate doesn't start until after submit

Yeah, it doesn't matter here. And fillIn is basically doing more as explained elsewhere (triggering input and change events), so when even fillIn does not make errors show up, then input would neither.

nicolechung

Thanks for catching this!

ynotdraw

Changes look great to me! Looks like a lingering @validateOn="blur" which is causing CI to be red

 error TS2322: Type '"blur"' is not assignable to type 'ValidateOn | undefined'.

ynotdraw · 2023-02-06T18:42:13Z

test-app/tests/helpers/dom.ts

+ * This mimics the behavior of a user typing data into an input without yet focusing out of it. Browsers will only trigger a `change` event when focusing
+ * out of the element, not while typing!
+ *
+ * `fillIn` will basically simulate entering the data *and* kinda focusing out (as it triggers `change`, however not `focusout`, which is impossible to achieve as a real user),


ynotdraw · 2023-02-06T18:50:12Z

test-app/tests/integration/components/headless-form-validation-test.gts

+            'validation errors are not rendered before initial validation happens before form is filled in'
+          );
+
+        await fillIn('[data-test-first-name]', 'Foo');


I think @simonihmig is doing something similar here as the above tests - ensuring fillIn doesn't cause errors to render, but below on line 1976 where he's using input it does

simonihmig · 2023-02-07T10:50:39Z

Changes look great to me! Looks like a lingering @validateOn="blur" which is causing CI to be red

Right, thank you! Fixed that. Good to see that Glint is actually working 🙂

simonihmig requested review from NullVoxPopuli, nicolechung and ynotdraw February 6, 2023 17:52

nicolechung reviewed Feb 6, 2023

View reviewed changes

NullVoxPopuli reviewed Feb 6, 2023

View reviewed changes

nicolechung reviewed Feb 6, 2023

View reviewed changes

test-app/tests/integration/components/headless-form-validation-test.gts Outdated Show resolved Hide resolved

nicolechung reviewed Feb 6, 2023

View reviewed changes

nicolechung approved these changes Feb 6, 2023

View reviewed changes

ynotdraw reviewed Feb 6, 2023

View reviewed changes

simonihmig added 3 commits February 7, 2023 11:48

Add `@validateOn="input", rename "blur" to "focusout"

a467090

Fix tests/linting

2bbaa70

Cleanup tests

b099152

simonihmig force-pushed the revalidate-input branch from 67e9fef to b099152 Compare February 7, 2023 10:49

ynotdraw approved these changes Feb 7, 2023

View reviewed changes

simonihmig merged commit 0f49c29 into main Feb 7, 2023

simonihmig deleted the revalidate-input branch February 7, 2023 12:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `@validateOn="input", rename "blur" to "focusout" #31

Add `@validateOn="input", rename "blur" to "focusout" #31

simonihmig commented Feb 6, 2023

changeset-bot bot commented Feb 6, 2023 •

edited

Loading

nicolechung commented Feb 6, 2023

NullVoxPopuli commented Feb 6, 2023 •

edited

Loading

nicolechung Feb 6, 2023

ynotdraw Feb 6, 2023

simonihmig Feb 7, 2023

simonihmig Feb 7, 2023

NullVoxPopuli Feb 6, 2023

nicolechung Feb 6, 2023

simonihmig Feb 7, 2023

nicolechung Feb 6, 2023

nicolechung Feb 6, 2023

simonihmig Feb 7, 2023

nicolechung Feb 6, 2023

ynotdraw Feb 6, 2023

nicolechung Feb 6, 2023

simonihmig Feb 7, 2023

nicolechung left a comment

ynotdraw left a comment

ynotdraw Feb 6, 2023

ynotdraw Feb 6, 2023

simonihmig commented Feb 7, 2023

Add `@validateOn="input", rename "blur" to "focusout" #31

Add `@validateOn="input", rename "blur" to "focusout" #31

Conversation

simonihmig commented Feb 6, 2023

changeset-bot bot commented Feb 6, 2023 • edited Loading

⚠️ No Changeset found

nicolechung commented Feb 6, 2023

NullVoxPopuli commented Feb 6, 2023 • edited Loading

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

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

nicolechung left a comment

Choose a reason for hiding this comment

ynotdraw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonihmig commented Feb 7, 2023

changeset-bot bot commented Feb 6, 2023 •

edited

Loading

NullVoxPopuli commented Feb 6, 2023 •

edited

Loading