feat: add overrideInputMode prop for screenreader accessibility (#463) - #465
feat: add overrideInputMode prop for screenreader accessibility (#463)#465dikshit-n wants to merge 5 commits into
Conversation
|
Do you want me to review this PR? Please comment |
|
/review |
|
PR Summary: Add overrideInputMode prop to control the inputMode attribute (fixes screenreader "stepper"/telephone announcements). Changes:
Notes:
|
|
Reviewed up to commit:e498c433dc4376e70ed8ad2beab22d668caa2738 |
|
Both points addressed in commit 517d936: Critical bug fixed: The expression now maps Nitpick fixed: The example no longer initializes |
|
/review |
|
Reviewed up to commit:517d936a2c42b1c3bceb09ead929cb9ffeaa57f6 Additional Suggestionexample/src/App.tsx, line:102-109The new uses `value={overrideInputMode}` while overrideInputMode can be undefined which can trigger controlled/uncontrolled React warnings. Provide an explicit default option representing the "input-type-driven" behaviour (e.g. value="" or value={undefined}) and bind the select with `value={overrideInputMode ?? ''}` (or set a sensible default like 'numeric'). Also consider adding options for all AllowedInputMode values or a "Default (input-type-driven)" option so the playground demonstrates all cases. <div className="side-bar__segment"> <label htmlFor="overrideInputMode">overrideInputMode</label> <select id="overrideInputMode" name="overrideInputMode" value={overrideInputMode ?? ''} onChange={handleChange} > <option value="">input-type-driven (default)</option> <option value="numeric">numeric — shows number pad on mobile</option> <option value="text">text — avoids screenreader stepper announcement</option> <option value="none">none — disables inputMode attribute</option> <option value="tel">tel</option> <option value="email">email</option> <option value="url">url</option> <option value="decimal">decimal</option> <option value="search">search</option> </select> </div> example/src/App.tsx, line:127 You forward `overrideInputMode={overrideInputMode}` to . Ensure the playground's initial value and select include an explicit 'default' state for the input-type-driven behavior (see previous comment) so users can revert to the original default behavior from the UI. // inside initial state overrideInputMode: undefined, // select binding <select id="overrideInputMode" name="overrideInputMode" value={overrideInputMode ?? ''} onChange={handleChange} > <option value="">input-type-driven (default)</option> {/* other options */} </select> // forwarding to OTPInput remains the same <OTPInput // ...other props overrideInputMode={overrideInputMode} renderInput={(props) => <input {...props} />} shouldAutoFocus /> src/index.tsx, line:269-271 The expression `overrideInputMode === 'none' ? undefined : overrideInputMode ?? (isInputNum ? 'numeric' : 'text')` correctly omits the attribute when 'none' is chosen. Consider making the intent clearer by extracting to a small helper (e.g. computeInputMode()) or adding a brief inline comment: `// 'none' -> omit attribute, undefined -> use inputType-driven default` to improve readability. const computeInputMode = ( overrideInputMode: AllowedInputMode | undefined, isInputNum: boolean ): InputModeValue => { // 'none' -> omit attribute, undefined -> use inputType-driven default if (overrideInputMode === 'none') return undefined; if (overrideInputMode) return overrideInputMode; return isInputNum ? 'numeric' : 'text'; }; // usage inputMode: computeInputMode(overrideInputMode, isInputNum), example/src/App.tsx, line:14 The initializer `overrideInputMode: undefined as unknown as string` is a type hack and confusing. Replace it with a properly typed value (e.g. `overrideInputMode: undefined as AllowedInputMode | undefined`) or include an explicit default (see next comment) so the intent is clear and TypeScript types are correct. const [{ otp, numInputs, separator, minLength, maxLength, placeholder, inputType, overrideInputMode }, setConfig] = React.useState<PlaygroundConfig>({ otp: '', numInputs: 4, separator: '-', minLength: 0, maxLength: 40, placeholder: '', inputType: 'text', overrideInputMode: undefined, }); Reviewed by Panto AI |
…ps type, typed PlaygroundConfig, README examples
|
All remaining review comments addressed in commit 911d7ae: Round 2 changes:
|
|
/review |
|
Reviewed up to commit:911d7ae1df5f4765edbac39e027458b00f3961ca Additional Suggestionexample/src/App.tsx, line:32-35handleChange uses a generic `[name]: value` assignment which is not type-safe for the strongly-typed PlaygroundConfig state. Prefer narrowing `name` to `keyof PlaygroundConfig` (or create separate handlers per field) so TypeScript can validate assignments. For example, type the event handler as `React.ChangeEvent & { target: HTMLInputElement & { name: keyof PlaygroundConfig } }` or cast `name as keyof PlaygroundConfig` when updating state.const handleChange = (
event: React.ChangeEvent<HTMLInputElement | HTMLSelectElement> & {
target: (EventTarget & HTMLInputElement) & { name: keyof PlaygroundConfig };
}
) => {
const { name, value } = event.target;
setConfig((prevConfig) => ({ ...prevConfig, [name]: value }));
}; |
… options, README none-attribute note
|
Addressed in commit aac652d:
|
|
/review |
|
Reviewed up to commit:aac652d033e0704098f8cf46b336816471807c0b Additional Suggestionsrc/index.tsx, line:249-281The mapping for inputMode uses overrideInputMode === 'none' ? undefined : overrideInputMode ?? (isInputNum ? 'numeric' : 'text'), which is correct. Consider moving this logic into a small helper (getInputMode) to keep renderInput call site concise and easier to test. Example: const getInputMode = (override?) => override === 'none' ? undefined : override ?? (isInputNum ? 'numeric' : 'text'); then pass inputMode: getInputMode(overrideInputMode).const getInputMode = (
overrideInputMode: AllowedInputMode | undefined,
isInputNum: boolean
): React.InputHTMLAttributes<HTMLInputElement>['inputMode'] => {
if (overrideInputMode === 'none') return undefined;
if (overrideInputMode != null) return overrideInputMode;
return isInputNum ? 'numeric' : 'text';
};
// usage inside map:
inputMode: getInputMode(overrideInputMode, isInputNum), |
…leChange numeric coercion, type-only import, README default clarity
|
All addressed in commit bdfbe47:
|
Summary
Add a new
overrideInputModeprop to let users control theinputModeHTML attribute independently ofinputType, fixing screenreader accessibility issues.Problem
When
inputTypeis set tonumberortel, the component setsinputMode="numeric"on each input. Screenreaders then announce the fields as "stepper" or "telephone" instead of a generic OTP input — see issue #463.Solution
Introduce
overrideInputMode?: "none" | "text" | "numeric" | "tel" | "email" | "url" | "decimal" | "search".inputType) for backward compatibility.overrideInputMode="text"withinputType="number"to keep number-only validation while eliminating the screenreader announcement issue.overrideInputMode="none"to omit the attribute entirely.Changes Made
overrideInputModeprop toOTPInputProps, destructured in the component, used to overrideinputModein the rendered inputs. Also exported the newAllowedInputModetype.overrideInputModein the example playground.inputMode.yarn install.Testing
tsc --noEmit).Checklist