refactor: conditionally inserting invalid message depending on validity#5850
refactor: conditionally inserting invalid message depending on validity#5850
Conversation
🦋 Changeset detectedLatest commit: cd130cc The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
that was a misunderstanding. |
|
@sarahbrng is there CSS code that could get removed by this change ? |
…placeholder-elements-when-omitted
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…placeholder-elements-when-omitted
…placeholder-elements-when-omitted Resolved merge conflicts: - Accepted main's version of all custom-select screenshot files (6 files) - Binary PNG files cannot be automatically merged - Screenshots will be regenerated during next visual regression test run
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…d-fields-that-add-todo-placeholder-elements-when-omitted # Conflicts: # __snapshots__/checkbox/patternhub/checkbox-properties-should-match-screenshot.png # __snapshots__/checkbox/showcase/mobile-chrome/DBCheckbox-should-match-screenshot-1/DBCheckbox-should-match-screenshot.png # __snapshots__/custom-select/showcase/chromium-highContrast/DBCustomSelect-should-match-screenshot-1/DBCustomSelect-should-match-screenshot.png # __snapshots__/custom-select/showcase/chromium/DBCustomSelect-should-match-screenshot-1/DBCustomSelect-should-match-screenshot.png # __snapshots__/custom-select/showcase/firefox/DBCustomSelect-should-match-screenshot-1/DBCustomSelect-should-match-screenshot.png # __snapshots__/custom-select/showcase/mobile-chrome/DBCustomSelect-should-match-screenshot-1/DBCustomSelect-should-match-screenshot.png # __snapshots__/input/showcase/chromium-highContrast/DBInput-should-match-screenshot-1/DBInput-should-match-screenshot.png # __snapshots__/input/showcase/chromium/DBInput-should-match-screenshot-1/DBInput-should-match-screenshot.png # __snapshots__/input/showcase/firefox/DBInput-should-match-screenshot-1/DBInput-should-match-screenshot.png # __snapshots__/input/showcase/mobile-chrome/DBInput-should-match-screenshot-1/DBInput-should-match-screenshot.png # __snapshots__/input/showcase/mobile-safari/DBInput-should-match-screenshot-1/DBInput-should-match-screenshot.png # __snapshots__/input/showcase/webkit/DBInput-should-match-screenshot-1/DBInput-should-match-screenshot.png # __snapshots__/select/showcase/chromium-highContrast/DBSelect-should-match-screenshot-1/DBSelect-should-match-screenshot.png # __snapshots__/select/showcase/chromium/DBSelect-should-match-screenshot-1/DBSelect-should-match-screenshot.png # __snapshots__/select/showcase/firefox/DBSelect-should-match-screenshot-1/DBSelect-should-match-screenshot.png # __snapshots__/select/showcase/mobile-chrome/DBSelect-should-match-screenshot-1/DBSelect-should-match-screenshot.png # __snapshots__/select/showcase/webkit/DBSelect-should-match-screenshot-1/DBSelect-should-match-screenshot.png # __snapshots__/switch/showcase/mobile-chrome/DBSwitch-should-match-screenshot-1/DBSwitch-should-match-screenshot.png # __snapshots__/textarea/showcase/chromium-highContrast/DBTextarea-should-match-screenshot-1/DBTextarea-should-match-screenshot.png # __snapshots__/textarea/showcase/chromium/DBTextarea-should-match-screenshot-1/DBTextarea-should-match-screenshot.png # __snapshots__/textarea/showcase/firefox/DBTextarea-should-match-screenshot-1/DBTextarea-should-match-screenshot.png # __snapshots__/textarea/showcase/mobile-chrome/DBTextarea-should-match-screenshot-1/DBTextarea-should-match-screenshot.png # __snapshots__/textarea/showcase/webkit/DBTextarea-should-match-screenshot-1/DBTextarea-should-match-screenshot.png # packages/components/src/components/checkbox/checkbox.lite.tsx # packages/components/src/components/input/input.lite.tsx # packages/components/src/components/select/select.lite.tsx # packages/components/src/components/switch/switch.lite.tsx # packages/components/src/components/textarea/textarea.lite.tsx
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…placeholder-elements-when-omitted
There was a problem hiding this comment.
Pull request overview
This PR removes placeholder "TODO" validation messages from form components by conditionally rendering invalid messages only when validation is actually active.
Changes:
- Removed
DEFAULT_INVALID_MESSAGEandDEFAULT_VALID_MESSAGEconstants that contained placeholder text - Added
hasInvalidState()method to validation state types - Wrapped invalid message rendering in
<Show when={state.hasInvalidState()}>conditionals across 6 components - Removed eager
_invalidMessageinitialization fromonMount()and redundantonUpdate()hooks - Updated test snapshots to reflect removed placeholder text
Reviewed changes
Copilot reviewed 14 out of 23 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/components/src/shared/model.ts | Added hasInvalidState() method to FromValidState type |
| packages/components/src/shared/constants.ts | Removed DEFAULT_INVALID_MESSAGE and DEFAULT_VALID_MESSAGE placeholder constants |
| packages/components/src/components/textarea/textarea.lite.tsx | Refactored to conditionally render invalid messages, removed eager initialization |
| packages/components/src/components/input/input.lite.tsx | Refactored to conditionally render invalid messages, removed eager initialization |
| packages/components/src/components/select/select.lite.tsx | Refactored to conditionally render invalid messages, removed eager initialization |
| packages/components/src/components/checkbox/checkbox.lite.tsx | Refactored to conditionally render invalid messages, removed eager initialization |
| packages/components/src/components/switch/switch.lite.tsx | Refactored to conditionally render invalid messages, removed eager initialization |
| packages/components/src/components/custom-select/custom-select.lite.tsx | Refactored to conditionally render invalid messages, removed eager initialization |
| snapshots/*.png | Updated visual regression test snapshots |
| snapshots/*.yaml | Updated accessibility tree snapshots removing placeholder text |
…placeholder-elements-when-omitted
| id={state._invalidMessageId} | ||
| size="small" | ||
| semantic="critical"> | ||
| {state._invalidMessage} |
There was a problem hiding this comment.
| {state._invalidMessage} | |
| {state._invalidMessage || props.invalidMessage} |
state._invalidMessage was removed from onMount and onUpdate, therefore, it is initially undefined and if validation is initially invalid (prop validation="invalid"), we would see an empty error-box Therefore, we should fall back to props.invalidMessage if the state is still empty. (for all components)
| {props.message} | ||
| </DBInfotext> | ||
| </Show> | ||
| <Show when={state.hasValidState()}> |
There was a problem hiding this comment.
| <Show when={state.hasValidState() && !!props.validMessage}> |
Gets shown if props.validation === 'valid'. If the developer didn't set props.validMessage, we get an empty green box.
…placeholder-elements-when-omitted
| @@ -142,12 +143,10 @@ export default function DBCustomSelect(props: DBCustomSelectProps) { | |||
| if (!selectRef?.validity.valid || props.validation === 'invalid') { | |||
There was a problem hiding this comment.
| if (!selectRef?.validity.valid || props.validation === 'invalid') { | |
| if (state.hasInvalidState()) { |
In all other components we use state.hasInvalidState(). I think it has been forgotten to be replaced here.
…d-fields-that-add-todo-placeholder-elements-when-omitted # Conflicts: # .config/cspellignorewords.txt
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…placeholder-elements-when-omitted
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Proposed changes
Fix Form Component Validation Message Display
Problem
Form components displayed placeholder validation messages ("TODO: Add an invalidMessage") even when validation wasn't active. Messages appeared with
display: none(hidden but present in DOM), creating unnecessary DOM nodes and potential accessibility concerns.Root Cause
Premature initialization of
_invalidMessageinonMount()and redundantonUpdate()hooks caused validation messages to be computed and rendered before actual validation occurred.Solution
_invalidMessageinitialization fromonMount()onUpdate()validation message hookshandleValidation()for lazy evaluationdisplay: noneworkaroundsValidation messages now only appear when validation actually runs.
resolves #5285
Types of changes
Further comments
🔭🐙🐈 Test this branch here: https://design-system.deutschebahn.com/core-web/review/5285-describe-required-fields-that-add-todo-placeholder-elements-when-omitted