CHI-3857: Hide validation errors until submit attempted#4272
CHI-3857: Hide validation errors until submit attempted#4272stephenhand merged 6 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the Pre-Engagement form UX so validation errors (inline messages + error styling) are hidden until the user attempts to submit the form.
Changes:
- Adds a
showErrorflag plumbed throughgenerateForm→ individual form input components. - Gates
hasErrorstyling and error message rendering in Input/Select/DependentSelect/Checkbox based onshowError. - Tracks
wasSubmitAttemptedinPreEngagementFormPhaseand passes it asshowError.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| aselo-webchat-react-app/src/components/forms/formInputs/index.tsx | Threads new showError option through form generation. |
| aselo-webchat-react-app/src/components/forms/formInputs/Input.tsx | Hides input error styling/message until showError is true. |
| aselo-webchat-react-app/src/components/forms/formInputs/Select.tsx | Hides select error styling/message until showError is true. |
| aselo-webchat-react-app/src/components/forms/formInputs/DependentSelect.tsx | Hides dependent select error styling/message until showError is true. |
| aselo-webchat-react-app/src/components/forms/formInputs/Checkbox.tsx | Hides checkbox error styling/message until showError is true. |
| aselo-webchat-react-app/src/components/PreEngagementFormPhase.tsx | Introduces wasSubmitAttempted state and passes it into generateForm. |
| getItem, | ||
| setItemValue, | ||
| showError: wasSubmitAttempted, | ||
| })} |
There was a problem hiding this comment.
This change alters when validation errors are displayed (hidden until a submit attempt). Please add/adjust tests to cover the new UI behavior (e.g., errors not rendered / hasError not set before submit, then shown after submit attempt), since existing tests mainly assert submission side effects rather than error visibility.
| const generateFormItem = ({ | ||
| definition, | ||
| defaultValue = '', | ||
| handleChange, | ||
| getItem, | ||
| setItemValue, | ||
| showError, | ||
| }: { |
There was a problem hiding this comment.
defaultValue is computed in generateForm and destructured in generateFormItem, but it isn't actually passed to any of the form input components here. Either forward it to the relevant inputs (e.g., Select/Checkbox) or remove the unused defaultValue plumbing (including getDefaultValue) to avoid misleading dead code.
| definition: PreEngagementFormItem; | ||
| defaultValue?: string | boolean; | ||
| handleChange: (payload: { name: string; value: string | boolean }) => void; | ||
| getItem: (inptuName: string) => PreEngagementDataItem; |
There was a problem hiding this comment.
Typo in the getItem callback type: parameter name is inptuName (missing 'u' in 'input'). Renaming to inputName will improve readability and avoid propagating the typo to other signatures.
| const setItemValue = ({ name }: { name: string }) => { | ||
| setFieldsTouched(fieldsTouched.add(name)); |
There was a problem hiding this comment.
setItemValue now ignores the value in the { name, value } payload. This breaks callers that rely on programmatic value updates (e.g., DependentSelect resets its child select via setItemValue({ name, value }) when the parent changes). Consider handling the passed value by dispatching an updatePreEngagementDataField(s) update for that specific field (or updating the DOM element before syncing) so dependent resets actually take effect.
| const setItemValue = ({ name }: { name: string }) => { | |
| setFieldsTouched(fieldsTouched.add(name)); | |
| const setItemValue = ({ name, value }: { name: string; value?: any }) => { | |
| setFieldsTouched(previousFieldsTouched => { | |
| const nextFieldsTouched = new Set(previousFieldsTouched); | |
| nextFieldsTouched.add(name); | |
| return nextFieldsTouched; | |
| }); | |
| if (typeof value !== 'undefined') { | |
| dispatch(updatePreEngagementDataFields([{ name, value }]) as any); | |
| return; | |
| } |
| {generateForm({ | ||
| form: preEngagementFormDefinition.fields, | ||
| handleChange, | ||
| getItem, | ||
| setItemValue, | ||
| showError: name => wasSubmitAttempted || fieldsTouched.has(name), | ||
| })} |
There was a problem hiding this comment.
This PR introduces new UI behavior (hide per-field validation errors until submit is attempted or the field is touched), but there are no tests asserting that errors are hidden initially and shown after touch/submit. Since PreEngagementFormPhase.test.tsx already has extensive validation coverage, please add a focused test (or two) to lock in the new showError gating behavior.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| const setItemValue = ({ name, value }: { name: string; value: string | boolean }) => { | ||
| setFieldsTouched(prevFieldsTouched => { | ||
| const nextFieldsTouched = new Set(prevFieldsTouched); |
There was a problem hiding this comment.
fieldsTouched is being mutated in-place (fieldsTouched.add(name)) and then passed back into state. Because the Set reference doesn't change, React may bail out and the component won't re-render, so showError may never update for touched fields. Use a functional state update that creates a new Set (e.g., copy prev then add) to preserve immutability and ensure re-rendering.
| let value: string | boolean; | ||
| if (updates[field.name]) { | ||
| value = updates[field.name]; | ||
| } else { | ||
| value = field.type === FormInputType.Checkbox ? (element as HTMLInputElement).checked : element.value; | ||
| } |
There was a problem hiding this comment.
setPreEngagementDataFromDom checks if (updates[field.name]), which ignores valid overrides like '' (empty string) or false (checkbox), and it will also skip overrides when the updated value is intentionally falsy. Check for key presence (e.g., field.name in updates / hasOwnProperty) or updates[field.name] !== undefined instead.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Already fixed in d8268cc — if (updates[field.name]) was changed to if (field.name in updates) so falsy overrides like '' and false are correctly applied.
| const setItemValue = ({ name, value }: { name: string; value: string | boolean }) => { | ||
| setFieldsTouched(prevFieldsTouched => { | ||
| const nextFieldsTouched = new Set(prevFieldsTouched); | ||
| nextFieldsTouched.add(name); |
There was a problem hiding this comment.
setPreEngagementDataFromDom({ name, value }) passes an object with keys literally named name and value, so updates[field.name] will never match the field being updated (unless a field is actually named "name"/"value"). If you intend to override a specific field, pass a record keyed by the field name (computed property) so the override is applied.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Fixed in d8268cc. Two bugs addressed:
setItemValuenow passes{ [name]: value }(computed property key) so the override object is keyed by the actual field name (e.g.{ friendlyName: 'John' }) rather than the literal{ name: 'friendlyName', value: 'John' }.setPreEngagementDataFromDomnow checksfield.name in updatesinstead ofif (updates[field.name])so falsy overrides like''(cleared input) orfalse(unchecked checkbox) are correctly applied rather than falling back to the DOM value.
| {generateForm({ | ||
| form: preEngagementFormDefinition.fields, | ||
| handleChange, |
There was a problem hiding this comment.
PR title/description says validation errors should only show after a submit attempt, but showError also enables errors as soon as a field is touched (wasSubmitAttempted || fieldsTouched.has(name)). If the intended behavior is strictly "after submit", drop the fieldsTouched condition (or adjust the PR description).
| <Text {...titleStyles} as="h3"> | ||
| <LocalizedTemplate code={titleText} /> | ||
| </Text> | ||
| <Box {...fieldStyles}> | ||
| {generateForm({ form: preEngagementFormDefinition.fields, handleChange, getItem, setItemValue })} | ||
| {generateForm({ | ||
| form: preEngagementFormDefinition.fields, | ||
| handleChange, |
There was a problem hiding this comment.
This change introduces new UI behavior (gating error rendering via showError), but there are no tests asserting that validation errors are hidden before the first submit attempt and become visible after submit. Please add/adjust tests in PreEngagementFormPhase.test.tsx to cover the new error-display timing so regressions are caught.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Added three focused tests in commit 778e42c:
- Errors hidden before submit — pre-loads Redux with a
RequiredFieldErrorstate and asserts the error span is not rendered before any interaction. - Errors shown after submit — submits the form with a required field empty and asserts the error span appears afterwards.
- Error shown after field is touched — blurs an empty required field (without submitting) and asserts the error span appears for that field only.
…romDom overrides Agent-Logs-Url: https://github.com/techmatters/flex-plugins/sessions/9fbd9699-ac66-4fff-84a9-7f6e62dbaee6 Co-authored-by: stephenhand <1694716+stephenhand@users.noreply.github.com>
Agent-Logs-Url: https://github.com/techmatters/flex-plugins/sessions/d718390f-adcf-45bb-83a8-0fd4de16994b Co-authored-by: stephenhand <1694716+stephenhand@users.noreply.github.com>
Description
Pre engagement form was showing validation errors too early. Now only shows them after a submit is attempted or after that specific field has been touched
Checklist
Other Related Issues
None
Verification steps
AFTER YOU MERGE
You are responsible for ensuring the above steps are completed. If you move a ticket into QA without advising what version to test, the QA team will assume the latest tag has the changes. If it does not, the following confusion is on you! :-P