Review pass & fixup of the control directive & related code #63068
Conversation
| readonly errors?: InputSignal<readonly ValidationError[] | undefined>; | ||
| // TODO: should we have an input for binding disabled reason? |
There was a problem hiding this comment.
TODO callout: Should we have an input for binding disabled reason?
| * will automatically bind the hidden status from the bound field to this input. | ||
| */ | ||
| readonly hidden?: InputSignal<boolean | undefined>; | ||
| // TODO: what do we do about the whole valid != invalid thing? |
There was a problem hiding this comment.
TODO callout: what do we do about the whole valid != !invalid thing?
- Should we bind both of them? neither? treat valid+pending as valid? some kind of combined tri-/quad-state?
- Should we allow binding in the pending state?
There was a problem hiding this comment.
I think we should do:
valid: false if there are any errors, true otherwisepending: true if there are any pending validators, false otherwise.
Notably this definition of valid is different than what FieldState.valid is, but I think it makes sense to convert it at this point to a value that makes more sense for the UI (which basically just want to know "do I show scary red or not?")
| * will automatically bind the value patterns from the bound field to this input. | ||
| */ | ||
| readonly pattern?: InputSignal<readonly RegExp[] | undefined>; | ||
| // TODO: what should we name this output? `touch` feels weird. The rest of the inputs here are |
There was a problem hiding this comment.
TODO callout: what should we name this output? touch feels weird. The rest of the inputs here are named after their corresponding DOM concept (when applicable). Following that pattern, blur might make sense, though maybe we consider this a separate thing that only happens to align with blur by default. Some alternative options:
- blur
- touchEvent
- touched (overlaps with the input name, could make it a
InputSignal | ModelSignal | OutputRef) - just remove this and don't allow customization of this behavior
| // TODO: Do we actually need to require the lack of a `checked` input? | ||
| // The implementation currently doesn't enforce this. It *does* enforce the lack of a `value` | ||
| // input for the `FormCheckboxControl` which should be sufficient to tell them apart. | ||
| // TODO: We currently require that a `checked` input not be present, as we may want to introduce a |
There was a problem hiding this comment.
TODO callout: We currently require that a checked input not be present, as we may want to introduce a third kind of form control for radio buttons that defines both a value and checked input. We are still evaluating whether this makes sense, but if we decide not to persue this we can remove this restriction.
cc @alxhub on this one, since he did some experimentation with it
| readonly checked: ModelSignal<boolean>; | ||
| // TODO: maybe this doesn't have to be strictly `undefined`? It just can't be a model signal. |
There was a problem hiding this comment.
TODO callout: maybe this doesn't have to be strictly undefined? It just can't be a model signal. Typescript doesn't really have a way to do any-but, but we could maybe introduce an optional generic for it?
| @@ -160,9 +193,11 @@ export class Control<T> { | |||
| input.addEventListener('blur', () => this.state().markAsTouched()); | |||
|
|
|||
| this.maybeSynchronize(() => this.state().readonly(), withBooleanAttribute(input, 'readonly')); | |||
| // TODO: consider making a global configuration option for using aria-disabled instead. | |||
There was a problem hiding this comment.
TODO callout: consider making a global configuration option for using aria-disabled instead.
| type NgControl, | ||
| type ValidationErrors, | ||
| type ValidatorFn, | ||
| } from '@angular/forms'; | ||
| import {REQUIRED} from '../api/property'; | ||
| import type {FieldState} from '../api/types'; | ||
|
|
||
| // TODO: Also consider supporting (if possible): |
There was a problem hiding this comment.
TODO callout: Also consider supporting (if possible):
- hasError
- getError
- reset
- name
- path
- markAs[Touched,Dirty,etc.]
- updateValueAndValidity
Also notes a few properties in comments that we should consider supporting if possible
35f39b4 to
b589dee
Compare
No description provided.