refactor: Allow setting's hint customization #37032
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughAdds a new optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin User
participant MS as MemoizedSetting
participant SI as SettingInput
participant FH as FieldHint
Admin->>MS: Open setting panel
MS->>SI: Render input with props (..., hint)
alt hint provided
SI->>FH: Render FieldHint with hint content
FH-->>SI: Displayed under input
else no hint
SI-->>Admin: Input rendered without FieldHint
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37032 +/- ##
===========================================
- Coverage 67.31% 67.30% -0.02%
===========================================
Files 3337 3337
Lines 113294 113326 +32
Branches 20562 20577 +15
===========================================
+ Hits 76266 76269 +3
- Misses 34422 34445 +23
- Partials 2606 2612 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/client/views/admin/settings/Setting/inputs/AssetSettingInput.tsx (1)
120-124: Fix accept attribute when no extensions are providedCurrent template emits ".undefined" when
fileConstraintsorextensionsare absent, unintentionally restricting uploads.- <input type='file' accept={`.${fileConstraints?.extensions?.join(', .')}`} onChange={handleUpload} disabled={disabled} /> + <input + type='file' + accept={fileConstraints?.extensions?.length ? `.${fileConstraints?.extensions?.join(', .')}` : undefined} + onChange={handleUpload} + disabled={disabled} + />apps/meteor/client/views/admin/settings/Setting/inputs/StringSettingInput.tsx (1)
7-11: Change SettingInputProps.hint to ReactNodeSettingInputProps currently declares
hint?: string— update it tohint?: ReactNodeto match consumers that pass React nodes (file: apps/meteor/client/views/admin/settings/Setting/inputs/types.ts; changehint?: string;→hint?: ReactNode;).
🧹 Nitpick comments (11)
apps/meteor/client/views/admin/settings/Setting/inputs/MultiSelectSettingInput.tsx (1)
18-59: Add aria-describedby to associate the hint with the input (accessibility).Link the hint to the control so screen readers announce it.
function MultiSelectSettingInput({ _id, label, value, - hint, + hint, placeholder, readonly, disabled, required, values = [], hasResetButton, onChangeValue, onResetButtonClick, autocomplete, }: MultiSelectSettingInputProps): ReactElement { const { t } = useTranslation(); const handleChange = (value: string[]): void => { onChangeValue?.(value); // onChangeValue && onChangeValue([...event.currentTarget.querySelectorAll('option')].filter((e) => e.selected).map((el) => el.value)); }; const Component = autocomplete ? MultiSelectFiltered : MultiSelect; + const hintId = `${_id}-hint`; return ( <Field> <FieldRow> <FieldLabel htmlFor={_id} title={_id} required={required}> {label} </FieldLabel> {hasResetButton && <ResetSettingButton data-qa-reset-setting-id={_id} onClick={onResetButtonClick} />} </FieldRow> <FieldRow> <Component max-width='full' data-qa-setting-id={_id} id={_id} value={value} placeholder={placeholder} disabled={disabled} readOnly={readonly} // autoComplete={autocomplete === false ? 'off' : undefined} onChange={handleChange} options={values.map(({ key, i18nLabel }) => [key, t(i18nLabel)])} + aria-describedby={hint ? hintId : undefined} /> </FieldRow> - {hint && <FieldHint>{hint}</FieldHint>} + {hint && <FieldHint id={hintId}>{hint}</FieldHint>} </Field> ); }apps/meteor/client/views/admin/settings/Setting/inputs/PasswordSettingInput.tsx (1)
13-48: Add aria-describedby to associate the hint with the input (accessibility).Ensure assistive tech reads the hint when focusing the password field.
function PasswordSettingInput({ _id, label, value, - hint, + hint, placeholder, readonly, autocomplete, disabled, required, hasResetButton, onChangeValue, onResetButtonClick, }: PasswordSettingInputProps): ReactElement { const handleChange: EventHandler<SyntheticEvent<HTMLInputElement>> = (event) => { onChangeValue?.(event.currentTarget.value); }; return ( <Field> <FieldRow> <FieldLabel htmlFor={_id} title={_id} required={required}> {label} </FieldLabel> {hasResetButton && <ResetSettingButton data-qa-reset-setting-id={_id} onClick={onResetButtonClick} />} </FieldRow> <FieldRow> <PasswordInput data-qa-setting-id={_id} id={_id} value={value} placeholder={placeholder} disabled={disabled} readOnly={readonly} autoComplete={autocomplete === false ? 'new-password' : undefined} onChange={handleChange} + aria-describedby={hint ? `${_id}-hint` : undefined} /> </FieldRow> - {hint && <FieldHint>{hint}</FieldHint>} + {hint && <FieldHint id={`${_id}-hint`}>{hint}</FieldHint>} </Field> ); }apps/meteor/client/views/admin/settings/Setting/MemoizedSetting.tsx (1)
90-101: Optional: tighten typing for InputComponent props to include hint.inputsByType uses ElementType, so TS won’t catch missing hint support. Consider a shared prop interface with hint?: ReactNode to improve safety.
apps/meteor/client/views/admin/settings/Setting/inputs/ColorSettingInput.tsx (1)
21-104: Add aria-describedby to associate the hint with the input(s) (accessibility).Apply to both editor modes and the trailing FieldHint.
function ColorSettingInput({ _id, label, value, - hint, + hint, editor, allowedTypes = [], placeholder, readonly, autocomplete, disabled, required, hasResetButton, onChangeValue, onChangeEditor, onResetButtonClick, }: ColorSettingInputProps): ReactElement { const { t } = useTranslation(); + const hintId = `${_id}-hint`; ... - {editor === 'color' && ( + {editor === 'color' && ( <InputBox data-qa-setting-id={_id} type='color' id={_id} value={value} placeholder={placeholder} disabled={disabled} readOnly={readonly} autoComplete={autocomplete === false ? 'off' : undefined} onChange={handleChange} + aria-describedby={hint ? hintId : undefined} /> )} - {editor === 'expression' && ( + {editor === 'expression' && ( <TextInput data-qa-setting-id={_id} id={_id} value={value} placeholder={placeholder} disabled={disabled} readOnly={readonly} autoComplete={autocomplete === false ? 'off' : undefined} onChange={handleChange} + aria-describedby={hint ? hintId : undefined} /> )} ... - {hint && <FieldHint>{hint}</FieldHint>} + {hint && <FieldHint id={hintId}>{hint}</FieldHint>}apps/meteor/client/views/admin/settings/Setting/inputs/LanguageSettingInput.tsx (1)
14-52: Add aria-describedby to associate the hint with the select (accessibility).function LanguageSettingInput({ _id, label, value, - hint, + hint, placeholder, readonly, autocomplete, disabled, required, hasResetButton, onChangeValue, onResetButtonClick, }: LanguageSettingInputProps): ReactElement { const languages = useLanguages(); + const hintId = `${_id}-hint`; ... <Select data-qa-setting-id={_id} id={_id} value={value} placeholder={placeholder} disabled={disabled} readOnly={readonly} autoComplete={autocomplete === false ? 'off' : undefined} onChange={(value) => handleChange(String(value))} options={languages.map(({ key, name }) => [key, name])} + aria-describedby={hint ? hintId : undefined} /> </FieldRow> - {hint && <FieldHint>{hint}</FieldHint>} + {hint && <FieldHint id={hintId}>{hint}</FieldHint>}apps/meteor/client/views/admin/settings/Setting/inputs/LookupSettingInput.tsx (1)
19-65: Add aria-describedby to associate the hint with the select (accessibility).function LookupSettingInput({ _id, label, value, - hint, + hint, placeholder, readonly, autocomplete, disabled, required, lookupEndpoint, hasResetButton, onChangeValue, onResetButtonClick, }: LookupSettingInputProps): ReactElement { + const hintId = `${_id}-hint`; ... <Select data-qa-setting-id={_id} id={_id} value={value} placeholder={placeholder} disabled={disabled} readOnly={readonly} autoComplete={autocomplete === false ? 'off' : undefined} onChange={(value) => handleChange(String(value))} options={values.map(({ key, label }) => [key, label])} + aria-describedby={hint ? hintId : undefined} /> </FieldRow> - {hint && <FieldHint>{hint}</FieldHint>} + {hint && <FieldHint id={hintId}>{hint}</FieldHint>}apps/meteor/client/views/admin/settings/Setting/inputs/SelectTimezoneSettingInput.tsx (1)
14-50: Add aria-describedby to associate the hint with the select (accessibility).function SelectTimezoneSettingInput({ _id, label, value, - hint, + hint, placeholder, readonly, autocomplete, disabled, required, hasResetButton, onChangeValue, onResetButtonClick, }: SelectTimezoneSettingInputProps): ReactElement { const handleChange = (value: string): void => { onChangeValue?.(value); }; + const hintId = `${_id}-hint`; ... <Select data-qa-setting-id={_id} id={_id} value={value} placeholder={placeholder} disabled={disabled} readOnly={readonly} autoComplete={autocomplete === false ? 'off' : undefined} onChange={(value) => handleChange(String(value))} options={moment.tz.names().map((key) => [key, key])} + aria-describedby={hint ? hintId : undefined} /> </FieldRow> - {hint && <FieldHint>{hint}</FieldHint>} + {hint && <FieldHint id={hintId}>{hint}</FieldHint>}apps/meteor/client/views/admin/settings/Setting/inputs/RoomPickSettingInput.tsx (1)
15-51: Add aria-describedby to associate the hint with the autocomplete (accessibility).If RoomAutoCompleteMultiple forwards aria-* props, wire it to the hint.
function RoomPickSettingInput({ _id, label, value, - hint, + hint, placeholder, readonly, disabled, required, hasResetButton, onChangeValue, onResetButtonClick, }: RoomPickSettingInputProps): ReactElement { const parsedValue = (value || []).map(({ _id }) => _id); + const hintId = `${_id}-hint`; ... <RoomAutoCompleteMultiple readOnly={readonly} placeholder={placeholder} disabled={disabled} value={parsedValue} onChange={handleChange} + aria-describedby={hint ? hintId : undefined} /> </FieldRow> - {hint && <FieldHint>{hint}</FieldHint>} + {hint && <FieldHint id={hintId}>{hint}</FieldHint>}apps/meteor/client/views/admin/settings/Setting/inputs/StringSettingInput.tsx (1)
44-56: Link hint to inputs via aria-describedby for better a11yAssociate FieldHint to the input/textarea so screen readers announce it.
<TextAreaInput data-qa-setting-id={_id} id={_id} name={name} rows={4} value={value} placeholder={placeholder} disabled={disabled} readOnly={readonly} error={error} autoComplete={autocomplete === false ? 'off' : undefined} + aria-describedby={hint ? `${_id}-hint` : undefined} onChange={handleChange} /> ... <TextInput data-qa-setting-id={_id} id={_id} value={value} name={name} placeholder={placeholder} disabled={disabled} readOnly={readonly} autoComplete={autocomplete === false ? 'off' : undefined} error={error} + aria-describedby={hint ? `${_id}-hint` : undefined} onChange={handleChange} /> ... - {hint && <FieldHint>{hint}</FieldHint>} + {hint && <FieldHint id={`${_id}-hint`}>{hint}</FieldHint>}Also applies to: 58-69, 72-73
apps/meteor/client/views/admin/settings/Setting/inputs/ActionSettingInput.tsx (1)
35-41: A11y: associate hints with the control via aria-describedbyImprove screen-reader experience by linking the hints to the button.
Apply within the changed range:
- {hint && <FieldHint>{hint}</FieldHint>} + {hint && <FieldHint id={`${_id}-hint`}>{hint}</FieldHint>}Apply outside the changed range (illustrative TSX snippet):
// before return (inside component) const describedBy = [ sectionChanged ? `${_id}-sectionChanged-hint` : undefined, hint ? `${_id}-hint` : undefined, ].filter(Boolean).join(' ') || undefined; // in JSX <Button data-qa-setting-id={_id} disabled={disabled || sectionChanged} primary onClick={handleClick} aria-describedby={describedBy} > {t(actionText)} </Button> {sectionChanged && <FieldHint id={`${_id}-sectionChanged-hint`}>{t('Save_to_enable_this_action')}</FieldHint>}apps/meteor/client/views/admin/settings/Setting/inputs/FontSettingInput.tsx (1)
49-49: A11y: link hint to input via aria-describedbyAssociate the hint with the input for screen readers.
Apply within the changed range:
- {hint && <FieldHint>{hint}</FieldHint>} + {hint && <FieldHint id={`${_id}-hint`}>{hint}</FieldHint>}Apply outside the changed range (illustrative TSX snippet):
// before return (inside component) const describedBy = hint ? `${_id}-hint` : undefined; // on TextInput <TextInput ... aria-describedby={describedBy} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (18)
apps/meteor/client/views/admin/settings/Setting/MemoizedSetting.tsx(1 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/ActionSettingInput.tsx(2 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/AssetSettingInput.tsx(3 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/BooleanSettingInput.tsx(3 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/ColorSettingInput.tsx(2 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/FontSettingInput.tsx(3 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/GenericSettingInput.tsx(3 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/IntSettingInput.tsx(3 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/LanguageSettingInput.tsx(3 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/LookupSettingInput.tsx(3 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/MultiSelectSettingInput.tsx(3 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/PasswordSettingInput.tsx(3 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/RelativeUrlSettingInput.tsx(3 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/RoomPickSettingInput.tsx(3 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/SelectSettingInput.tsx(3 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/SelectTimezoneSettingInput.tsx(3 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/StringSettingInput.tsx(3 hunks)apps/meteor/client/views/admin/settings/Setting/inputs/TimespanSettingInput.tsx(3 hunks)
🔇 Additional comments (11)
apps/meteor/client/views/admin/settings/Setting/MemoizedSetting.tsx (1)
90-101: All InputComponents render the hint prop.
Confirmed: every input referenced by MemoizedSetting renders FieldHint (checked apps/meteor/client/views/admin/settings/Setting/inputs).apps/meteor/client/views/admin/settings/Setting/inputs/IntSettingInput.tsx (1)
38-49: Add aria-describedby to tie FieldHint to the input (duplicate of StringSettingInput)This improves screen reader support.
<InputBox data-qa-setting-id={_id} id={_id} type='number' value={value} placeholder={placeholder} disabled={disabled} readOnly={readonly} autoComplete={autocomplete === false ? 'off' : undefined} + aria-describedby={hint ? `${_id}-hint` : undefined} onChange={handleChange} /> ... - {hint && <FieldHint>{hint}</FieldHint>} + {hint && <FieldHint id={`${_id}-hint`}>{hint}</FieldHint>}Also applies to: 50-51
apps/meteor/client/views/admin/settings/Setting/inputs/GenericSettingInput.tsx (1)
38-47: Add aria-describedby to tie FieldHint to the input (duplicate of StringSettingInput)Aligns a11y behavior across inputs.
<TextInput data-qa-setting-id={_id} id={_id} value={value} placeholder={placeholder} disabled={disabled} readOnly={readonly} autoComplete={autocomplete === false ? 'off' : undefined} + aria-describedby={hint ? `${_id}-hint` : undefined} onChange={handleChange} /> ... - {hint && <FieldHint>{hint}</FieldHint>} + {hint && <FieldHint id={`${_id}-hint`}>{hint}</FieldHint>}Also applies to: 49-50
apps/meteor/client/views/admin/settings/Setting/inputs/SelectSettingInput.tsx (2)
43-53: Add aria-describedby to tie FieldHint to the select (duplicate of StringSettingInput)Improves hint discoverability for assistive tech.
<Select data-qa-setting-id={_id} id={_id} value={value} placeholder={placeholder} disabled={disabled} readOnly={readonly} autoComplete={autocomplete === false ? 'off' : undefined} + aria-describedby={hint ? `${_id}-hint` : undefined} onChange={(value) => handleChange(String(value))} options={values.map(({ key, i18nLabel }) => [key, t(i18nLabel)])} /> ... - {hint && <FieldHint>{hint}</FieldHint>} + {hint && <FieldHint id={`${_id}-hint`}>{hint}</FieldHint>}Also applies to: 55-56
17-27: Resolved — MemoizedSetting forwards hint and no longer renders FieldHint; inputs render their own hint MemoizedSetting passes hint down to InputComponent and does not render FieldHint; each input in apps/meteor/client/views/admin/settings/Setting/inputs accepts a hint prop and renders FieldHint, so no duplicate hint rendering.apps/meteor/client/views/admin/settings/Setting/inputs/AssetSettingInput.tsx (1)
120-124: Add aria-describedby to tie FieldHint to the file input (duplicate of StringSettingInput)Improves a11y for the upload control.
- <input type='file' accept={`.${fileConstraints?.extensions?.join(', .')}`} onChange={handleUpload} disabled={disabled} /> + <input + type='file' + accept={fileConstraints?.extensions?.length ? `.${fileConstraints?.extensions?.join(', .')}` : undefined} + onChange={handleUpload} + disabled={disabled} + aria-describedby={hint ? `${_id}-hint` : undefined} + /> ... - {hint && <FieldHint>{hint}</FieldHint>} + {hint && <FieldHint id={`${_id}-hint`}>{hint}</FieldHint>}Also applies to: 128-129
apps/meteor/client/views/admin/settings/Setting/inputs/TimespanSettingInput.tsx (1)
94-104: Add aria-describedby to tie FieldHint to both inputs (duplicate of StringSettingInput)Covers number input and unit select.
<InputBox data-qa-setting-id={_id} id={_id} type='number' value={internalValue} placeholder={placeholder} disabled={disabled} readOnly={readonly} autoComplete={autocomplete === false ? 'off' : undefined} + aria-describedby={hint ? `${_id}-hint` : undefined} onChange={handleChange} /> </FieldRow> <FieldRow> - <Select value={timeUnit} disabled={disabled} options={timeUnitOptions} onChange={handleChangeTimeUnit} /> + <Select + value={timeUnit} + disabled={disabled} + options={timeUnitOptions} + onChange={handleChangeTimeUnit} + aria-describedby={hint ? `${_id}-hint` : undefined} + /> </FieldRow> - {hint && <FieldHint>{hint}</FieldHint>} + {hint && <FieldHint id={`${_id}-hint`}>{hint}</FieldHint>}Also applies to: 107-108, 109-110
apps/meteor/client/views/admin/settings/Setting/inputs/BooleanSettingInput.tsx (1)
34-41: Add aria-describedby to tie FieldHint to the ToggleSwitch (duplicate of StringSettingInput)Improves screen reader output.
<ToggleSwitch data-qa-setting-id={_id} id={_id} checked={value === true} disabled={disabled || readonly} + aria-describedby={hint ? `${_id}-hint` : undefined} onChange={handleChange} /> ... - {hint && <FieldHint>{hint}</FieldHint>} + {hint && <FieldHint id={`${_id}-hint`}>{hint}</FieldHint>}Also applies to: 43-44
apps/meteor/client/views/admin/settings/Setting/inputs/RelativeUrlSettingInput.tsx (1)
38-47: Add aria-describedby to tie FieldHint to the UrlInput (duplicate of StringSettingInput)Helps assistive tech announce the hint.
<UrlInput data-qa-setting-id={_id} id={_id} value={getAbsoluteUrl(value || '')} placeholder={placeholder} disabled={disabled} readOnly={readonly} autoComplete={autocomplete === false ? 'off' : undefined} + aria-describedby={hint ? `${_id}-hint` : undefined} onChange={handleChange} /> - {hint && <FieldHint>{hint}</FieldHint>} + {hint && <FieldHint id={`${_id}-hint`}>{hint}</FieldHint>}Also applies to: 48-49
apps/meteor/client/views/admin/settings/Setting/inputs/ActionSettingInput.tsx (1)
12-16: Prop added + rendering LGTMAccepting
hint?: ReactNodeand rendering it viaFieldHintis consistent with the new API and keeps the existing “save to enable” hint intact.Also applies to: 41-41
apps/meteor/client/views/admin/settings/Setting/inputs/FontSettingInput.tsx (1)
1-1: Hint rendering LGTMAdding
FieldHintand wiring it to the existinghintprop matches the new pattern.Also applies to: 49-49
apps/meteor/client/views/admin/settings/Setting/inputs/ActionSettingInput.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/admin/settings/Setting/inputs/ActionSettingInput.tsx
Show resolved
Hide resolved
MartinSchoeler
left a comment
MartinSchoeler
left a comment
There was a problem hiding this comment.
NVM I forgot that we provide the setting key as an ID on the tooltips
MartinSchoeler
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Since our settings inputs already receive the hint prop, it makes sense to allow them to handle the
FieldHintinternally, allowing customization as neededIssue(s)
Steps to test or reproduce
Further comments
CORE-1397
Summary by CodeRabbit
New Features
Style