-
Notifications
You must be signed in to change notification settings - Fork 21
feat(gcds-select): Add native HTML attributes and validation #1013
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
base: main
Are you sure you want to change the base?
Conversation
| this.internals.setFormValue(null); | ||
| this.value = null; | ||
| } | ||
| console.log('value changed to: ', this.value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| console.log('value changed to: ', this.value); |
Co-authored-by: Daine Trinidad <[email protected]>
Co-authored-by: Daine Trinidad <[email protected]>
Co-authored-by: Daine Trinidad <[email protected]>
| /** | ||
| * If true, the select will be focused on component render | ||
| */ | ||
| @Prop({ reflect: true }) autofocus: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be optional too? And/or have an inferred default on declaration?
| @Prop({ reflect: true }) autofocus: boolean; | |
| @Prop({ reflect: true }) autofocus?: false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems the build fails when adding a ? to the autofocus property. Luckily the property is still marked as optional even if it doesn't have a ?
| ${args.disabled ? `disabled` : null} | ||
| ${args.autocomplete ? `autocomplete="${args.autocomplete}"` : null} | ||
| ${args.autofocus ? `autofocus` : null} | ||
| ${args.form ? `form="${args.value}"` : null} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be
| ${args.form ? `form="${args.value}"` : null} | |
| ${args.form ? `form="${args.form}"` : null} |
| ${args.disabled ? `disabled` : null} | ||
| ${args.autocomplete ? `autocomplete="${args.autocomplete}"` : null} | ||
| ${args.autofocus ? `autofocus` : null} | ||
| ${args.form ? `form="${args.value}"` : null} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ${args.form ? `form="${args.value}"` : null} | |
| ${args.form ? `form="${args.form}"` : null} |
…c/gcds-components into feat/gcds-select-validity
daine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Summary | Résumé
Add attributes to match
gcds-selectbehaviour to the HTMLselectto provide additional validation.gcds-selecthas also been expanded to work with the HTML ValidityState to provide developers with more options when validatinggcds-selects and new methods to check current validation state.New properties/attributes
autofocusautofocusbooleanundefinedformformstringundefinedvalidityvalidityValidityStateundefinedNew methods
cds-snc/design-gc-conception#2022