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

Jump to bottom

Support @validateOn="blur/change" #24

Merged
merged 2 commits into from
Feb 2, 2023
Merged

Support @validateOn="blur/change" #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2ee44c1
Validate fields on blur/change
simonihmig Jan 27, 2023
f7a640c
Add tests for `@validateOn="change"`
simonihmig Jan 27, 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
1 change: 1 addition & 0 deletions ember-headless-form/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
"@types/ember__debug": "^4.0.0",
"@types/ember__engine": "^4.0.0",
"@types/ember__error": "^4.0.0",
"@types/ember__modifier": "^4.0.3",
"@types/ember__object": "^4.0.0",
"@types/ember__polyfills": "^4.0.0",
"@types/ember__routing": "^4.0.0",
Expand Down
8 changes: 3 additions & 5 deletions ember-headless-form/src/components/-private/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export type FieldValidateCallback<
* Internal structure to track used fields
* @private
*/
export interface FieldData<
export interface FieldRegistrationData<
DATA extends FormData,
KEY extends FormKey<DATA> = FormKey<DATA>
> {
Expand All @@ -69,7 +69,7 @@ export interface FieldData<
export type RegisterFieldCallback<
DATA extends FormData,
KEY extends FormKey<DATA> = FormKey<DATA>
> = (name: KEY, field: FieldData<DATA, KEY>) => void;
> = (name: KEY, field: FieldRegistrationData<DATA, KEY>) => void;

export type UnregisterFieldCallback<
DATA extends FormData,
Expand All @@ -79,6 +79,4 @@ export type UnregisterFieldCallback<
/**
* Mapper type to construct subset of objects, whose keys are only strings (and not number or symbol)
*/
export type OnlyStringKeys<T extends object> = {
[P in Extract<keyof T, string>]: T[P];
};
export type OnlyStringKeys<T extends object> = Pick<T, keyof T & string>;
11 changes: 9 additions & 2 deletions ember-headless-form/src/components/headless-form.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,18 @@
<form ...attributes {{on 'submit' this.onSubmit}}>
{{! ignoring prettier here is need to *not* wrap the modifier usage below into a new line, making @glint-expect-error fail to work 🙈 }}
{{! prettier-ignore }}
<form
...attributes
{{on 'submit' this.onSubmit}}
{{! @glint-expect-error: modifier helper not supported, see https://github.com/typed-ember/glint/issues/410 }}
{{(if this.fieldValidationEvent (modifier this.on this.fieldValidationEvent this.handleFieldValidation))}}
>
{{yield
(hash
field=(component
(ensure-safe-component this.FieldComponent)
data=this.internalData
set=this.set
errors=this.lastValidationResult
errors=this.visibleErrors
registerField=this.registerField
unregisterField=this.unregisterField
)
Expand Down
86 changes: 79 additions & 7 deletions ember-headless-form/src/components/headless-form.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import Component from '@glimmer/component';
import { tracked } from '@glimmer/tracking';
import { assert } from '@ember/debug';
import { on } from '@ember/modifier';
import { action } from '@ember/object';

import { TrackedObject } from 'tracked-built-ins';
Expand All @@ -10,11 +11,13 @@ import FieldComponent from './-private/field';
import type { HeadlessFormFieldComponentSignature } from './-private/field';
import type {
ErrorRecord,
FieldData,
FieldRegistrationData,
FieldValidateCallback,
FormData,
FormKey,
FormValidateCallback,
UserData,
ValidationError,
} from './-private/types';
import type { ComponentLike, WithBoundArgs } from '@glint/template';

Expand All @@ -37,27 +40,42 @@ export interface HeadlessFormComponentSignature<DATA extends UserData> {
default: [
{
field: WithBoundArgs<
typeof FieldComponent<FormData<DATA>>,
typeof FieldComponent<DATA>,
'data' | 'set' | 'errors' | 'registerField' | 'unregisterField'
>;
}
];
};
}

class FieldData<
DATA extends FormData,
KEY extends FormKey<DATA> = FormKey<DATA>
> {
constructor(fieldRegistration: FieldRegistrationData<DATA, KEY>) {
this.validate = fieldRegistration.validate;
}

@tracked validationEnabled = false;

validate?: FieldValidateCallback<DATA, KEY>;
}

export default class HeadlessFormComponent<
DATA extends UserData
> extends Component<HeadlessFormComponentSignature<DATA>> {
FieldComponent: ComponentLike<HeadlessFormFieldComponentSignature<DATA>> =
FieldComponent;

internalData: FormData<DATA> = new TrackedObject(
this.args.data ?? {}
) as FormData<DATA>;
// we cannot use (modifier "on") directly in the template due to https://github.com/emberjs/ember.js/issues/19869
on = on;

internalData: DATA = new TrackedObject(this.args.data ?? {}) as DATA;

fields = new Map<FormKey<FormData<DATA>>, FieldData<FormData<DATA>>>();

@tracked lastValidationResult?: ErrorRecord<FormData<DATA>>;
@tracked showAllValidations = false;

get validateOn(): ValidateOn {
return this.args.validateOn ?? 'submit';
Expand All @@ -67,6 +85,16 @@ export default class HeadlessFormComponent<
return this.args.revalidateOn ?? 'change';
}

get fieldValidationEvent(): 'focusout' | 'change' | undefined {
const { validateOn } = this;

return validateOn === 'submit'
? undefined
: validateOn === 'blur'
? 'focusout'
: validateOn;
}

get hasValidationErrors(): boolean {
// Only consider validation errors for which we actually have a field rendered
return this.lastValidationResult
Expand Down Expand Up @@ -106,11 +134,38 @@ export default class HeadlessFormComponent<
return Object.keys(errors).length > 0 ? errors : undefined;
}

get visibleErrors(): ErrorRecord<FormData<DATA>> | undefined {
if (!this.lastValidationResult) {
return undefined;
}

const visibleErrors: ErrorRecord<FormData<DATA>> = {};

for (const [field, errors] of Object.entries(this.lastValidationResult) as [
FormKey<FormData<DATA>>,
ValidationError<FormData<DATA>[FormKey<FormData<DATA>>]>[]
][]) {
if (this.showErrorsFor(field)) {
visibleErrors[field] = errors;
}
}

return visibleErrors;
}

showErrorsFor(field: FormKey<FormData<DATA>>): boolean {
return (
this.showAllValidations ||
(this.fields.get(field)?.validationEnabled ?? false)
);
}

@action
async onSubmit(e: Event): Promise<void> {
e.preventDefault();

this.lastValidationResult = await this.validate();
this.showAllValidations = true;

if (!this.hasValidationErrors) {
this.args.onSubmit?.(this.internalData);
Expand All @@ -126,15 +181,15 @@ export default class HeadlessFormComponent<
@action
registerField(
name: FormKey<FormData<DATA>>,
field: FieldData<FormData<DATA>>
field: FieldRegistrationData<FormData<DATA>>
): void {
assert(
`You passed @name="${String(
name
)}" to the form field, but this is already in use. Names of form fields must be unique!`,
!this.fields.has(name)
);
this.fields.set(name, field);
this.fields.set(name, new FieldData(field));
}

@action
Expand All @@ -146,4 +201,21 @@ export default class HeadlessFormComponent<
set<KEY extends FormKey<FormData<DATA>>>(key: KEY, value: DATA[KEY]): void {
this.internalData[key] = value;
}

@action
async handleFieldValidation(e: Event): Promise<void> {
const { target } = e;
const { name } = target as HTMLInputElement;

if (name) {
const field = this.fields.get(name as FormKey<FormData<DATA>>);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the name matching logic of variant 1 happening!


if (field) {
this.lastValidationResult = await this.validate();
field.validationEnabled = true;
}
} else {
// @todo how to handle custom controls that don't emit focusout/change events from native form controls?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yeah, that's the downside of that approach 😬

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

imo,
custom controls can / should emit these events tho.

"To be a custom control, it must behave like a control, including having <these events>"

Copy link
Contributor

@ynotdraw ynotdraw Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

imo,
custom controls can / should emit these events tho.

Yeah, I agree. We definitely will want validation to trigger for controls like <PowerSelect, <PowerCalendar, etc. used inside of forms. Getting these elements to act more like their native versions would be 💯 (e.g., <PowerSelect -> <select>, <PowerCalendar -> <input type="date", etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, but the question is still whether to do 1. or 3. then.

For 1. to work, the event must have event.target.name === @name passed to field. A <PowerSelect> could have <input type="hidden" name={{@name}}> (does it, idk?), and if that triggers focusout then we are fine.

But what about the example with the 3-select date picker. It will receive @name="date", but will split this up internally to 3 selects of names date_day, date_month, date_year. When one of those selects triggers a focusout, our form will not know what field this belongs to, because event.target.name === 'date'.

For 3., event.target.name would not matter, and everything would "just work"(TM). But the downside (wrapper div) remains...

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the caveat for 1 is fine.
For 3, can you write out the details of this approach by extending this demo -- i just want to be super extra sure I understand how this wrapping div approach would work

Copy link
Contributor Author

@simonihmig simonihmig Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure tbh how the demo would relate to this addon, but I'll try it this way:

The current form.field template would have a wrapping div like so:

<div {{on "focusout (fn @triggerValidationFor @name)}} ...attributes>
  {{! here is what is currently in the template }}
</div>

@triggerValidationFor would be curried into the component when yielding it as a contextual component, so that's a private implementation detail.

From the caller's site:

<HeadlessForm as |form|>
  <form.field @name="date" as |field|>  <-- this would now be a div that captures the event, and *knows* what @name it belongs to!
    <field.label>Date:</field.label>
    <CustomDatePickerComponentConsistingOfThreeSelectsWithUnknownNames
      @value={{field.value}}
      @onUpdate={{field.setValue}}
    />
    <field.errors/>
  </form.field>
</HeadlessForm>

So here, as long as the custom component triggers a focusout event, validation would work ootb, no matter if there is no native form element, or what name it/they have or not have...

Btw, triggerValidationFor would not call e.stopPropagation(), so the event would not stop to bubble at the form.field div level, if that was a concern. You could still attach event listeners to the <form> or even parent elements, and see the event...

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

maybe this div is too much to assume. I imagine this may be something that a custom-control author would want to opt in to.

as in:

<HeadlessForm as |form|>
  <form.field @name="date" as |field|> 
    <field.captureEvents @events={{"focusout"}}> or @events={{array "focusout" "change"}} ? idk
     ^-- this would now be a div that captures the event, and *knows* what @name it belongs to!
    <field.label>Date:</field.label>
      <CustomDatePickerComponentConsistingOfThreeSelectsWithUnknownNames
        @value={{field.value}}
        @onUpdate={{field.setValue}}
      />
    </field.captureEvents>
    <field.errors/>
  </form
</HeadlessForm>

But maybe that begins to defeat the purpose of the headless form?

would be curried into the component when yielding it as a contextual component, so that's a private implementation detail.

would there be a way to not add those event bindings if no validator is passed? (conditionally apply the on modifier?)

Btw, triggerValidationFor would not call e.stopPropagation(), so the event would not stop to bubble at the form.field div level, if that was a concern

it was a concern! this is good news.

You could still attach event listeners to the

or even parent elements, and see the event...

most excellent.

I think I've now changed my opinion from being all in on option 1, to at least being a bit 50/50 on options 1 and 3 now.

I do have a concern though, so, what would happen if a field has multiple focusables? (this may sway me back to option 1? idk!)

<HeadlessForm as |form|>
  <form.field @name="date" as |field|>
    <field.label>
       Date:
       <button {{on 'click' (fn this.helpAbout "date")}}>help</button>
        ^ - would we end up validating on events from here?
    </field.label>
    <CustomDatePickerComponentConsistingOfThreeSelectsWithUnknownNames
      @value={{field.value}}
      @onUpdate={{field.setValue}}
    />
    <field.errors/>
  </form
</HeadlessForm>

}
}
}
22 changes: 14 additions & 8 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion test-app/app/templates/index.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
<HeadlessForm @data={{this.data}} @onSubmit={{this.doSomething}} as |form|>
<HeadlessForm
@data={{this.data}}
@validateOn='blur'
@onSubmit={{this.doSomething}}
as |form|
>
<form.field @name='name' as |field|>
<field.label>Name</field.label>
<field.input />
Expand Down
Loading