Add validation and accessibility to NumberField expiry inputs#364
Conversation
- Add clamping in handleChange: end_sequence min 1, end_shot 0-30 - Use NumberField's label prop for proper accessibility (htmlFor/id) - Reuse handleChange instead of inline handlers to reduce duplication Addresses review comments from PR #363.
There was a problem hiding this comment.
Pull request overview
This PR tightens validation and improves UI for the expiry fields in AddEffectModal, while reusing the shared handleChange helper and using NumberField’s label support for better UX/accessibility.
Changes:
- Updated
handleChangeto clampend_sequenceto a minimum of 1 andend_shotto the 0–30 range, handling empty values asnull. - Switched the expiry UI to use
NumberFieldwithlabelprops and the sharedhandleChangeinstead of inline handlers and separate captionTypographylabels.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| label="End Sequence" | ||
| onChange={handleChange("end_sequence")} | ||
| onBlur={() => {}} | ||
| /> | ||
| <NumberField | ||
| name="end_shot" | ||
| value={effect.end_shot ?? ""} | ||
| size="small" | ||
| width="100px" | ||
| error={false} | ||
| label="End Shot" | ||
| onChange={handleChange("end_shot")} |
There was a problem hiding this comment.
Using the label prop here improves the visual labeling of the expiry fields, but our current NumberField implementation (src/components/ui/NumberField.tsx) renders an InputLabel that is not actually associated with the inner TextField via htmlFor/id, so this does not yet provide a proper programmatic label for screen readers as implied in the PR description. To fully achieve the intended accessibility improvement, we should update NumberField to wire the label to the input (e.g., generate a stable id for the input and pass htmlFor on InputLabel) or adjust the description to avoid over-claiming the a11y behavior.
Summary
Addresses review comments from PR #363:
handleChangefor numeric fields:end_sequence: minimum value of 1end_shot: clamped to 0-30 rangelabelprop which properly associates the label with the input viahtmlFor/idhandleChangefunction instead of inline handlers to reduce duplicationTest plan