-
Notifications
You must be signed in to change notification settings - Fork 76
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
feat: use input-message to display validation messages for invalid fields after form submission #8574
Conversation
…idation-ui * origin/main: refactor: remove @ts-ignore needed for Stencil missing in JSX types (#8551) refactor(combobox): remove unused interface (#8552) fix(input-date-picker): ensure range icon toggles open corresponding date-picker (#8554) fix(button): avoid needlessly overwriting title (#8491)
expect(await isElementFocused(page, hiddenInputSelector)).toBe(true); | ||
expect(await isElementFocused(page, inputSelector)).toBe(false); | ||
expect(await isElementFocused(page, hiddenInputSelector)).toBe(false); | ||
expect(await isElementFocused(page, inputSelector)).toBe(true); |
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.
@jcfranco this test was added in #8563, do you remember if there was a reason that the hidden input needs to be focused after form submission? Or were these expects just added for context and the real thing being tested is that typing after submitting actually changes the value?
The latter is still working with this change and I added tests for it in form.e2e.ts
.
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.
do you remember if there was a reason that the hidden input needs to be focused after form submission?
Yes, it's because the browser will focus and display the validation message on the hidden input. This is no longer the case with your changes, right? 🚀
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.
yup!
"calcite-input-text", | ||
"calcite-text-area", | ||
]; | ||
|
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.
We need to make sure to update this array if additional Input components are added
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.
We could look at introducing a common interface that would allow us to determine this dynamically without an allowlist.
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.
Interesting, could you elaborate on this?
} | ||
|
||
// prevent the browser from showing the native validation popover | ||
event?.preventDefault(); |
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.
Note to future self: moving this preventDefault to before the conditional on line 183 will suppress the native validation popover that is displayed for non-calcite elements.
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.
👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍
👍✅👍👍👍✅👍✅👍👍👍✅👍👍✅✅✅👍👍✅✅✅👍👍✅✅✅👍✅👍
👍👍✅👍✅👍👍✅👍👍👍✅👍✅👍👍👍👍✅👍👍👍👍✅👍👍👍👍✅👍
👍👍👍✅👍👍👍✅👍👍👍✅👍👍✅✅👍👍👍✅✅👍👍👍✅✅👍👍✅👍
👍👍👍✅👍👍👍✅👍👍👍✅👍👍👍👍✅👍👍👍👍✅👍👍👍👍✅👍👍👍
👍👍👍✅👍👍👍👍✅✅✅👍👍✅✅✅👍👍✅✅✅👍👍✅✅✅👍👍✅👍
👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍
"calcite-input-text", | ||
"calcite-text-area", | ||
]; | ||
|
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.
We could look at introducing a common interface that would allow us to determine this dynamically without an allowlist.
import { html } from "../../support/formatting"; | ||
import { componentsWithInputEvent } from "./form"; | ||
|
||
describe("form", () => { |
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.
We could tackle this separately, but could we generalize these assertions and move them over to the formAssociated
test helper?
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.
Not all form-associated components have validation (e.g. slider, switch, meter, etc.), and IIRC you mentioned in the past that you don't like conditional tests/assertions.
I'd also have to add new options for stuff like:
- the value to type to ensure the Change event fires (differs for input-date-picker, input-time-picker, input-number, etc.)
- A boolean for components like radio-button-group which need to have a child clicked rather than a value typed.
I'd be happy to make the change if you're okay with the conditional tests and additional options, just let me know.
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.
Alternatively I could make a validation common test helper or create some utils to reduce the repeat code in form.e2e.ts
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.
IIRC you mentioned in the past that you don't like conditional tests/assertions.
It's been a while, but maybe I brought it up in the context of individual, multipurpose tests? 🤔💭❓For the common test helpers, we do have conditional testing depending on what they are meant to cover (see example).
I'd also have to add new options for stuff like:
Makes sense. We do have options for some of these specialized ones (see example).
Alternatively I could make a validation common test helper or create some utils to reduce the repeat code in form.e2e.ts
It'd be great to have utils that we can use within the formAssociated
test helper.
const hiddenInput = event?.target as HTMLInputElement; | ||
|
||
// not necessarily a calcite-input, but we don't have an HTMLCalciteFormElement type | ||
const formComponent = hiddenInput?.parentElement as HTMLCalciteInputElement; |
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.
We could work on adding this custom type.
…idation-ui * origin/main: feat(handle, block, list-item): improve drag handle tooltip to include item label (#8584) chore: release next feat: reflect validationIcon property (#8583) chore: release next docs: improve consistency for component prop descriptions (#8580) test(action-menu): skip close on blur test. (#8585) feat(tabs): emit selection-related events when selection is modified after closing the selected tab (#8582)
…idation-ui * origin/main: chore: release next fix(text-area): prevent infinite render loop when `max-length` property is defined (#8610)
🤖 I have created a release *beep* *boop* --- <details><summary>@esri/calcite-design-tokens: 2.1.1</summary> ## [2.1.1](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected]) (2024-01-17) ### Bug Fixes * Allow users to control tabindex on interactive components ([#8166](#8166)) ([b15c052](b15c052)) ### Reverts * Chore(modal): remove e2e tests that are covered by dedicated openClose commonTests helper ([#8392](#8392)) ([#8471](#8471)) ([4bedf99](4bedf99)) </details> <details><summary>@esri/calcite-components: 2.2.0</summary> ## [2.2.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected]) (2024-01-17) ### Features * **action-menu:** Close menu on blur instead of on tab key. ([#8577](#8577)) ([ccfbd0c](ccfbd0c)) * **checkbox, combobox, input-date-picker, input-time-picker, segmented-control, select:** Add required property ([#8517](#8517)) ([72a1ce4](72a1ce4)) * **handle, block, list-item:** Improve drag handle tooltip to include item label ([#8584](#8584)) ([6e643e2](6e643e2)) * **handle:** Add `blurUnselectDisabled` property to disable unselecting handle on blur. ([#8483](#8483)) ([4d665cc](4d665cc)) * **handle:** Add selected property and calciteHandleChange event. ([#8484](#8484)) ([d2e9880](d2e9880)) * **list-item:** Add dragSelected property and calciteListItemDragHandleChange event ([#8524](#8524)) ([4db2eb7](4db2eb7)) * **list-item:** Add tooltip for expanding and collapsing ([#8612](#8612)) ([4964491](4964491)) * **list:** Add "filter-no-results" slot to display content when no filtered items are shown ([#8569](#8569)) ([f1fc7f6](f1fc7f6)) * **list:** Introduce clearer unselected state ([#8510](#8510)) ([f1e836c](f1e836c)) * **radio-button-group, segmented control:** Add validationMessage, validationIcon, and status properties ([#8561](#8561)) ([d4c5efc](d4c5efc)) * Reflect validationIcon property ([#8583](#8583)) ([b3d38b3](b3d38b3)) * **table-header:** Add style when within a `selected` Table Row ([#8449](#8449)) ([13cfe75](13cfe75)) * **tabs:** Emit selection-related events when selection is modified after closing the selected tab ([#8582](#8582)) ([b15c940](b15c940)) * **tile:** Add visual scales ([#8496](#8496)) ([7638ec4](7638ec4)) * Use input-message to display validation messages for invalid fields after form submission ([#8574](#8574)) ([fd392fe](fd392fe)) ### Bug Fixes * **action:** Update component tokens to support transparent ([#8532](#8532)) ([81cb5cc](81cb5cc)) * Allow users to control tabindex on interactive components ([#8166](#8166)) ([b15c052](b15c052)) * **button:** Avoid needlessly overwriting title ([#8491](#8491)) ([350a983](350a983)) * **color-picker:** Emit color change when nudging color channels by using the shift key ([#8579](#8579)) ([4250598](4250598)) * **combobox:** Only allow deleting visible chips with the keyboard ([#8603](#8603)) ([2d38241](2d38241)) * **date-picker:** Prevent console error when selecting just an end date for input date picker ([#8444](#8444)) ([c0e51c3](c0e51c3)) * **filter:** Prevent console warning from displaying to end users ([#8458](#8458)) ([0de7646](0de7646)) * **input-date-picker:** Ensure range icon toggles open corresponding date-picker ([#8554](#8554)) ([cfafd15](cfafd15)) * **input-date-picker:** Resolve a hard to reproduce number formatter caching issue that occurred due to the countdown delay in queued Alerts. ([5f4fa3e](5f4fa3e)) * **input-message:** Add missing margin to scale="s", spacing CSS variable has effect ([#8592](#8592)) ([49b0a20](49b0a20)) * **input, input-number, input-text:** Restore focus on input after browser validation error is displayed and user continues typing ([#8563](#8563)) ([5897965](5897965)) * **input, input-number:** Support setting value property to Infinity ([#8547](#8547)) ([f6ac698](f6ac698)) * **list-item:** Store last focused cell from focusing on elements within a cell. ([#8494](#8494)) ([28f93b4](28f93b4)) * **list, list-item, list-item-group:** Honor hidden attribute on list-item and list-item-group ([#8541](#8541)) ([3851dc6](3851dc6)) * **list:** Correct selectedItems value when list is filtered ([#8481](#8481)) ([9de1922](9de1922)) * **list:** Fix event detail newIndex when down arrow pressed to sort ([#8462](#8462)) ([b3d5169](b3d5169)) * **list:** Fix keyboard arrow navigation ([#8470](#8470)) ([57fdaa4](57fdaa4)) * **modal:** Ensure focus trapping in dynamically created, subsequently opened modals ([#8593](#8593)) ([4ec6b94](4ec6b94)) * **table:** Fix double border on `bordered` Table Rows in `table-footer` ([#8509](#8509)) ([c16ea33](c16ea33)) * **table:** Improve Table overflow behavior ([#8424](#8424)) ([79743e1](79743e1)) * **text-area:** Prevent infinite render loop when `max-length` property is defined ([#8610](#8610)) ([f30d933](f30d933)) ### Reverts * Chore(modal): remove e2e tests that are covered by dedicated openClose commonTests helper ([#8392](#8392)) ([#8471](#8471)) ([4bedf99](4bedf99)) ### Dependencies * The following workspace dependencies were updated * devDependencies * @esri/calcite-design-tokens bumped from ^2.1.1-next.4 to ^2.1.1 </details> <details><summary>@esri/calcite-components-angular: 2.2.0</summary> ## [2.2.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected]) (2024-01-17) ### Miscellaneous Chores * **@esri/calcite-components-angular:** Synchronize undefined versions ### Dependencies * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^2.2.0-next.22 to ^2.2.0 </details> <details><summary>@esri/calcite-components-react: 2.2.0</summary> ## [2.2.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected]) (2024-01-17) ### Miscellaneous Chores * **@esri/calcite-components-react:** Synchronize undefined versions ### Dependencies * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^2.2.0-next.22 to ^2.2.0 </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…form submission (#8690) **Related Issue:** #8000 ## Summary - Only set `validationMessage` property during form submission if it doesn't have an existing value. This allows users to set a custom validation message, which will be displayed instead of the default message provided by the browser. - Move the form validation e2e tests to the formAssociated common test, which was [requested as a follow up item](#8574 (comment)).
…form submission (#8690) **Related Issue:** #8000 ## Summary - Only set `validationMessage` property during form submission if it doesn't have an existing value. This allows users to set a custom validation message, which will be displayed instead of the default message provided by the browser. - Move the form validation e2e tests to the formAssociated common test, which was [requested as a follow up item](#8574 (comment)).
Related Issue: #8000
Summary
calcite-input-message
used to display the validation message is cleared once the component's Change event fires (or Input event forcalcite-input
,calcite-input-number
,calcite-input-text
, andcalcite-text-area
).calcite-input-message
to prevent UI inconsistencies.calcite-input-message
under calcite form elements with invalid values.