Skip to content

Improve setup deck selection UX#5211

Merged
matthewevans merged 2 commits into
mainfrom
ship/improve-setup-deck-selection-ux
Jul 6, 2026
Merged

Improve setup deck selection UX#5211
matthewevans merged 2 commits into
mainfrom
ship/improve-setup-deck-selection-ux

Conversation

@matthewevans

Copy link
Copy Markdown
Member

No description provided.

@matthewevans matthewevans enabled auto-merge July 6, 2026 21:37

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces a random deck selection feature at match start, adds filters for deck selection by source and precon set, and implements a new CounterTooltip component for localized counter descriptions. It also enhances the ChooseXValueUI component with manual numeric input and stepper buttons. Feedback on the UI changes highlights issues with the input clamping logic: clamping immediately on every keystroke prevents users from typing values that temporarily fall below the minimum limit. The reviewer suggests clamping only the upper bound during input, applying full clamping on blur, and defensively clamping values before performing stepper increments or decrements.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +58 to +64
const handleValueChange = useCallback(
(nextValue: number) => {
if (!Number.isFinite(nextValue)) return;
setValue(clampValue(nextValue));
},
[clampValue],
);

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.

high

Clamping the value immediately on every keystroke makes it impossible for users to type numbers starting with a digit less than the first digit of min (e.g., typing 1 when min is 10 immediately clamps to 10, making it impossible to type 15). We should only clamp the upper bound immediately, and let the lower bound be clamped on blur or commit.

Suggested change
const handleValueChange = useCallback(
(nextValue: number) => {
if (!Number.isFinite(nextValue)) return;
setValue(clampValue(nextValue));
},
[clampValue],
);
const handleValueChange = useCallback(
(nextValue: number) => {
if (!Number.isFinite(nextValue)) return;
setValue(Math.min(nextValue, max));
},
[max],
);

Comment on lines +128 to +141
<button
type="button"
onClick={() => handleValueChange(value - 1)}
disabled={value <= min}
aria-label={t("mana.decreaseX")}
className={gameButtonClass({
tone: "neutral",
size: "xs",
disabled: value <= min,
className: "h-9 w-9 px-0 text-base",
})}
>
</button>

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.

high

Defensively clamp the current value before decrementing to ensure correct behavior even if the user is in the middle of typing an out-of-bounds or empty value.

Suggested change
<button
type="button"
onClick={() => handleValueChange(value - 1)}
disabled={value <= min}
aria-label={t("mana.decreaseX")}
className={gameButtonClass({
tone: "neutral",
size: "xs",
disabled: value <= min,
className: "h-9 w-9 px-0 text-base",
})}
>
</button>
<button
type="button"
onClick={() => handleValueChange(clampValue(value) - 1)}
disabled={clampValue(value) <= min}
aria-label={t("mana.decreaseX")}
className={gameButtonClass({
tone: "neutral",
size: "xs",
disabled: clampValue(value) <= min,
className: "h-9 w-9 px-0 text-base",
})}
>
</button>

Comment on lines +142 to +152
<input
type="number"
min={min}
max={max}
step={1}
inputMode="numeric"
value={value}
onChange={(e) => handleValueChange(Number(e.target.value))}
aria-label={t("mana.chooseXInputAria")}
className="h-9 w-20 rounded-lg border border-cyan-400/30 bg-gray-950/80 px-2 text-center font-mono text-base font-semibold text-cyan-100 shadow-inner outline-none transition focus:border-cyan-300 focus:ring-2 focus:ring-cyan-400/30"
/>

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.

high

Add an onBlur handler to ensure that the value is fully clamped to the [min, max] range when the user finishes typing.

              <input
                type="number"
                min={min}
                max={max}
                step={1}
                inputMode="numeric"
                value={value}
                onChange={(e) => handleValueChange(Number(e.target.value))}
                onBlur={() => setValue(clampValue(value))}
                aria-label={t("mana.chooseXInputAria")}
                className="h-9 w-20 rounded-lg border border-cyan-400/30 bg-gray-950/80 px-2 text-center font-mono text-base font-semibold text-cyan-100 shadow-inner outline-none transition focus:border-cyan-300 focus:ring-2 focus:ring-cyan-400/30"
              />

Comment on lines +153 to +166
<button
type="button"
onClick={() => handleValueChange(value + 1)}
disabled={value >= max}
aria-label={t("mana.increaseX")}
className={gameButtonClass({
tone: "neutral",
size: "xs",
disabled: value >= max,
className: "h-9 w-9 px-0 text-base",
})}
>
+
</button>

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.

high

Defensively clamp the current value before incrementing to ensure correct behavior even if the user is in the middle of typing an out-of-bounds or empty value.

Suggested change
<button
type="button"
onClick={() => handleValueChange(value + 1)}
disabled={value >= max}
aria-label={t("mana.increaseX")}
className={gameButtonClass({
tone: "neutral",
size: "xs",
disabled: value >= max,
className: "h-9 w-9 px-0 text-base",
})}
>
+
</button>
<button
type="button"
onClick={() => handleValueChange(clampValue(value) + 1)}
disabled={clampValue(value) >= max}
aria-label={t("mana.increaseX")}
className={gameButtonClass({
tone: "neutral",
size: "xs",
disabled: clampValue(value) >= max,
className: "h-9 w-9 px-0 text-base",
})}
>
+
</button>

@matthewevans matthewevans added this pull request to the merge queue Jul 6, 2026
@matthewevans matthewevans removed this pull request from the merge queue due to a manual request Jul 6, 2026
@matthewevans matthewevans enabled auto-merge July 6, 2026 22:00
@matthewevans matthewevans added this pull request to the merge queue Jul 6, 2026
Merged via the queue into main with commit 605b400 Jul 6, 2026
11 checks passed
@matthewevans matthewevans deleted the ship/improve-setup-deck-selection-ux branch July 6, 2026 22:39
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.

1 participant