EDM-4145: Common helperText for unique Name field#704
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
Walkthrough
ChangesHelper text and field text updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/ui-components/src/components/form/NameField.tsx`:
- Around line 17-20: The NameFieldHelperText component currently wraps the
translated helper text in a DefaultHelperText component, but this component is
then passed as the helperText prop to RichValidationTextField which wraps it
again in DefaultHelperText, causing double-wrapping that breaks PatternFly
styling. Modify NameFieldHelperText to return only the translated text string
(without the DefaultHelperText wrapper), allowing RichValidationTextField to
apply the wrapper once. This change affects both the NameFieldHelperText
function definition and where it is used as the helperText prop in
RichValidationTextField.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 17c669b4-3457-4f25-95b8-e283adfe6b98
⛔ Files ignored due to path filters (1)
libs/i18n/locales/en/translation.jsonis excluded by!libs/i18n/locales/en/translation.json
📒 Files selected for processing (7)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProviderForm.tsxlibs/ui-components/src/components/Catalog/AddCatalogItemWizard/CreateCatalogModal.tsxlibs/ui-components/src/components/Catalog/AddCatalogItemWizard/steps/GeneralInfoStep.tsxlibs/ui-components/src/components/Catalog/AddCatalogItemWizard/steps/TypeConfigStep.tsxlibs/ui-components/src/components/ImagePromotion/ImagePromotionForm.tsxlibs/ui-components/src/components/form/NameField.tsxlibs/ui-components/src/components/form/RichValidationTextField.tsx
💤 Files with no reviewable changes (1)
- libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProviderForm.tsx
celdrake
force-pushed
the
EDM-4145-name-display-helper-text
branch
from
d1cddc7 to
ad6d36a
Compare
jkilzi
left a comment
jkilzi
left a comment
There was a problem hiding this comment.
Good intent — standardising the helper text is the right call. One prop-ordering bug needs fixing before merge.
Accessibility wins — the additions in and friends are a nice bonus.
Nit — drops the "Optional display name" helper text from the artifact name field while also relabelling it "Display name". If the field is optional, some hint to that effect would still be helpful for users.
| validations={validations} | ||
| resourceType={resourceType} | ||
| helperText={t('A unique system identifier. Cannot be changed after creation.')} | ||
| {...rest} |
There was a problem hiding this comment.
{...rest} is spread after the explicit helperText prop, so any caller that passes helperText through rest will override the standardised string — defeating the purpose of this PR.
Move {...rest} before helperText so the hardcoded text always wins:
<NameField
name={name}
validations={validations}
resourceType={resourceType}
{...rest}
helperText={t('A unique system identifier. Cannot be changed after creation.')}
/>There was a problem hiding this comment.
You are right, fixed now.
Made-with: Cursor
Made-with: Cursor
celdrake
force-pushed
the
EDM-4145-name-display-helper-text
branch
from
ad6d36a to
6882310
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProviderForm.tsx`:
- Around line 57-65: The Button in CreateAuthProviderForm uses a hardcoded
user-facing aria-label, so it should be localized like the rest of the
component. Update the aria-label on the Button in CreateAuthProviderForm to use
t(...) instead of the literal "Enabled help text", keeping the label
context-aware and matching the existing translation patterns in this form.
In `@libs/ui-components/src/components/form/RichValidationTextField.tsx`:
- Line 157: The new helper text rendered by DefaultHelperText is not associated
with the input, so TextInput’s aria-describedby target does not resolve. Update
RichValidationTextField so the helper text element gets the same helper id
expected by TextInput (the fieldId-based helper identifier), ensuring the
guidance is exposed to screen readers. Use the existing symbols TextInput,
DefaultHelperText, and helperText/fieldId wiring to keep the association
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a6625b3a-5a1e-4117-8b7d-3b1a0b1758e6
⛔ Files ignored due to path filters (1)
libs/i18n/locales/en/translation.jsonis excluded by!libs/i18n/locales/en/translation.json
📒 Files selected for processing (7)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProviderForm.tsxlibs/ui-components/src/components/Catalog/AddCatalogItemWizard/CreateCatalogModal.tsxlibs/ui-components/src/components/Catalog/AddCatalogItemWizard/steps/GeneralInfoStep.tsxlibs/ui-components/src/components/Catalog/AddCatalogItemWizard/steps/TypeConfigStep.tsxlibs/ui-components/src/components/ImagePromotion/ImagePromotionForm.tsxlibs/ui-components/src/components/form/NameField.tsxlibs/ui-components/src/components/form/RichValidationTextField.tsx
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProviderForm.tsx`:
- Around line 57-65: The Button in CreateAuthProviderForm uses a hardcoded
user-facing aria-label, so it should be localized like the rest of the
component. Update the aria-label on the Button in CreateAuthProviderForm to use
t(...) instead of the literal "Enabled help text", keeping the label
context-aware and matching the existing translation patterns in this form.
In `@libs/ui-components/src/components/form/RichValidationTextField.tsx`:
- Line 157: The new helper text rendered by DefaultHelperText is not associated
with the input, so TextInput’s aria-describedby target does not resolve. Update
RichValidationTextField so the helper text element gets the same helper id
expected by TextInput (the fieldId-based helper identifier), ensuring the
guidance is exposed to screen readers. Use the existing symbols TextInput,
DefaultHelperText, and helperText/fieldId wiring to keep the association
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a6625b3a-5a1e-4117-8b7d-3b1a0b1758e6
⛔ Files ignored due to path filters (1)
libs/i18n/locales/en/translation.jsonis excluded by!libs/i18n/locales/en/translation.json
📒 Files selected for processing (7)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProviderForm.tsxlibs/ui-components/src/components/Catalog/AddCatalogItemWizard/CreateCatalogModal.tsxlibs/ui-components/src/components/Catalog/AddCatalogItemWizard/steps/GeneralInfoStep.tsxlibs/ui-components/src/components/Catalog/AddCatalogItemWizard/steps/TypeConfigStep.tsxlibs/ui-components/src/components/ImagePromotion/ImagePromotionForm.tsxlibs/ui-components/src/components/form/NameField.tsxlibs/ui-components/src/components/form/RichValidationTextField.tsx
🛑 Comments failed to post (2)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProviderForm.tsx (1)
57-65: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the new button aria-label.
aria-label="Enabled help text"is user-facing copy in a.tsxfile, so it should go throught(...)like the rest of this component. As per coding guidelines,**/*.tsx: "Flag user-visible strings in .tsx files that are not wrapped in the t() translation function." As per path instructions,{libs,apps}/*/src/**/*.{ts,tsx}: "All user-visible strings must use the t() translation function."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProviderForm.tsx` around lines 57 - 65, The Button in CreateAuthProviderForm uses a hardcoded user-facing aria-label, so it should be localized like the rest of the component. Update the aria-label on the Button in CreateAuthProviderForm to use t(...) instead of the literal "Enabled help text", keeping the label context-aware and matching the existing translation patterns in this form.Sources: Coding guidelines, Path instructions
libs/ui-components/src/components/form/RichValidationTextField.tsx (1)
157-157: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Associate the new helper text with the input.
This renders the helper copy visually, but
TextInputstill pointsaria-describedbyat${fieldId}-helperwhileDefaultHelperTextdoes not render that id. Screen readers will miss the new guidance.Suggested fix
- <DefaultHelperText helperText={helperText} /> + <DefaultHelperText id={`${fieldId}-helper`} helperText={helperText} />-export const DefaultHelperText = ({ helperText }: DefaultHelperTextProps) => { +export const DefaultHelperText = ({ id, helperText }: DefaultHelperTextProps) => { return helperText ? ( - <FormHelperText> + <FormHelperText id={id}> <HelperText> <HelperTextItem variant="default">{helperText}</HelperTextItem> </HelperText> </FormHelperText> ) : null; };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<DefaultHelperText id={`${fieldId}-helper`} helperText={helperText} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/ui-components/src/components/form/RichValidationTextField.tsx` at line 157, The new helper text rendered by DefaultHelperText is not associated with the input, so TextInput’s aria-describedby target does not resolve. Update RichValidationTextField so the helper text element gets the same helper id expected by TextInput (the fieldId-based helper identifier), ensuring the guidance is exposed to screen readers. Use the existing symbols TextInput, DefaultHelperText, and helperText/fieldId wiring to keep the association consistent.
2 participants
Show a unified helperText in the "Name" field.
Add a clarification for CatalogItem's "Display Name".
Areas Affected
libs/ui-components/ (Shared UI Components)
components/form/NameField.tsxcomponents/form/RichValidationTextField.tsxcomponents/Catalog/AddCatalogItemWizard/CreateCatalogModal.tsxcomponents/Catalog/AddCatalogItemWizard/steps/GeneralInfoStep.tsxcomponents/Catalog/AddCatalogItemWizard/steps/TypeConfigStep.tsxcomponents/ImagePromotion/ImagePromotionForm.tsxcomponents/AuthProvider/CreateAuthProvider/CreateAuthProviderForm.tsxCross-cutting Implications
NameField,RichValidationTextField) affect any app surfaces that consumeui-components(e.g., both standalone and any OCP plugin usage).Summary of Changes
This PR centralizes and unifies helper-text rendering for “Name” fields across the UI component library and clarifies “CatalogItem” “Display name” labeling.
Core component updates
NameFieldhelperTextprop and forwards it toRichValidationTextField.NameFieldWrapperso it always supplies the translated “A unique system identifier. Cannot be changed after creation.” helper text (replacing the prior conditional behavior where helper text was only rendered when provided/disabled).RichValidationTextFieldhelperText?: React.ReactNodeprop.DefaultHelperTextalongside existing input/validation UI.Consumer updates
NameFieldusage so it relies on the centralizedNameFieldbehavior.aria-label,helperText,isDisabled) are consistently applied.aria-label, helper text, and explicit disabled handling; also adds anaria-labelfor the “Readme” field.Internationalization
DefaultHelperText), ensuring consistent wording across the app.Non-goals / Explicitly out of scope
libs/types/,libs/i18n/(beyond consuming existing translation wiring in the shared UI components),libs/cypress/,apps/*, Go auth proxy, container builds, packaging, or CI/workflow configuration.