Skip to content

Use NumberField for expiry inputs in character effects dialog#363

Merged
progressions merged 1 commit into
masterfrom
feature/numberfield-character-effects
Jan 27, 2026
Merged

Use NumberField for expiry inputs in character effects dialog#363
progressions merged 1 commit into
masterfrom
feature/numberfield-character-effects

Conversation

@progressions

Copy link
Copy Markdown
Owner

Summary

  • Replace TextField type="number" with NumberField components for the end_sequence and end_shot fields in AddEffectModal
  • Matches the pattern used in AddFightEffectModal for consistency
  • NumberField provides increment/decrement buttons that slide in on hover for better UX

Test plan

  • Open an encounter with characters
  • Click the + button on a character to add an effect
  • Verify the End Sequence and End Shot fields show NumberField components with +/- buttons on hover
  • Verify incrementing/decrementing works correctly
  • Verify creating an effect with expiry values works

Replace TextField type="number" with NumberField components for the
end_sequence and end_shot fields, matching the pattern used in
AddFightEffectModal. NumberField provides increment/decrement buttons
that slide in on hover for better UX.
Copilot AI review requested due to automatic review settings January 27, 2026 19:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Replaces the expiry inputs in the character Add Effect modal with the shared NumberField component to align UI/UX with other effect dialogs and provide increment/decrement controls.

Changes:

  • Swap TextField type="number" for NumberField for end_sequence and end_shot.
  • Update layout around expiry inputs to use caption labels and fixed-width fields.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +248 to +260
<NumberField
name="end_sequence"
value={effect.end_sequence ?? ""}
size="small"
width="100px"
error={false}
onChange={(e: React.ChangeEvent<HTMLInputElement>) => {
const val = e.target.value
setEffect(prev => ({
...prev,
end_sequence: val === "" ? null : Number(val),
}))
}}

Copilot AI Jan 27, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change drops the previous numeric constraints (min: 1 for end_sequence). With NumberField’s +/- controls, users can now decrement into invalid values (e.g., 0 or negative) and submit them. Please reintroduce a lower bound (e.g., clamp in the onChange handler, or extend NumberField to accept/enforce min).

Copilot uses AI. Check for mistakes.
Comment on lines +254 to +260
onChange={(e: React.ChangeEvent<HTMLInputElement>) => {
const val = e.target.value
setEffect(prev => ({
...prev,
end_sequence: val === "" ? null : Number(val),
}))
}}

Copilot AI Jan 27, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The onChange handler here duplicates the existing handleChange("end_sequence") logic (which already handles nullable numeric fields). Reusing the shared handler would reduce duplication and keep numeric parsing behavior consistent across the modal.

Copilot uses AI. Check for mistakes.
Comment on lines +280 to +282
setEffect(prev => ({
...prev,
end_shot: val === "" ? null : Number(val),

Copilot AI Jan 27, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change drops the previous end_shot bounds (min: 0, max: 30). With NumberField’s +/- controls (and no max), users can now enter negative shots or values > 30 and submit them. Please enforce the expected range (clamp in onChange, or extend NumberField to support min/max and pass them through).

Suggested change
setEffect(prev => ({
...prev,
end_shot: val === "" ? null : Number(val),
if (val === "") {
setEffect(prev => ({
...prev,
end_shot: null,
}))
return
}
let num = Number(val)
if (Number.isNaN(num)) {
return
}
if (num < 0) {
num = 0
} else if (num > 30) {
num = 30
}
setEffect(prev => ({
...prev,
end_shot: num,

Copilot uses AI. Check for mistakes.
Comment on lines +241 to +252
<Typography
variant="caption"
color="text.secondary"
sx={{ mb: 0.5 }}
>
End Sequence
</Typography>
<NumberField
name="end_sequence"
value={effect.end_sequence ?? ""}
size="small"
width="100px"

Copilot AI Jan 27, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new NumberField inputs no longer have an associated form label (the Typography caption isn’t programmatically tied to the input), which is an accessibility regression vs the previous TextField label prop. Consider wiring an accessible label via label/id + htmlFor (or aria-label/aria-labelledby)—either by enhancing NumberField to expose these props properly, or by using a labeled MUI TextField under the hood.

Copilot uses AI. Check for mistakes.
@progressions progressions merged commit 8d6d698 into master Jan 27, 2026
10 checks passed
@progressions progressions deleted the feature/numberfield-character-effects branch January 27, 2026 20:01
progressions added a commit that referenced this pull request Jan 27, 2026
- 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.
progressions added a commit that referenced this pull request Jan 29, 2026
- 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants