Skip to content

Conversation

rutmanz
Copy link
Member

@rutmanz rutmanz commented Jul 22, 2025

Task

AARD-1945

Symptom

Previously, both "Use Controller" and "Use Touch Controls" could be enabled, in which case the UI would show the configuration for the controller. This doesn't really make sense because the use touch controls property is basically ignored (although still set in the JS object)
image

Solution

Updated checkboxes to allow passing your own state variable, while still keeping the old "stateless" option to avoid requiring extra code wherever checkboxes are used

Verification

Opened the input configuration panel and attempted to turn both on, and observed that while both could be off, turning on one would turn off the other. I also opened the robot switch panel to confirm that the index-based checkboxes still worked as intended. I also opened the preference panel to confirm that normal checkboxes worked correctly.


Before merging, ensure the following criteria are met:

  • All acceptance criteria outlined in the ticket are met.
  • Necessary test cases have been added and updated.
  • A feature toggle or safe disable path has been added (if applicable).
  • User-facing polish:
    • Ask: "Is this ready-looking?"
  • Cross-linking between Jira and GitHub:
    • PR links to the relevant Jira issue.
    • Jira ticket has a comment referencing this PR.

@rutmanz rutmanz requested review from a team as code owners July 22, 2025 17:00
@AlexD717 AlexD717 added the ui/ux Relating to user interface, or in general, user experience label Jul 22, 2025
Copy link
Member

@AlexD717 AlexD717 left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines -14 to +15
const [useGamepad, setUseGamepad] = useState<boolean>(selectedScheme.usesGamepad)
const [useTouchControls, setUseTouchControls] = useState<boolean>(selectedScheme.usesTouchControls)
const [useGamepad, setUseGamepad] = useState(selectedScheme.usesGamepad)
const [useTouchControls, setUseTouchControls] = useState(selectedScheme.usesTouchControls)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel as though keeping the will help with code readability

Copy link
Contributor

Choose a reason for hiding this comment

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

I think usesGamepad and usesTouchControls are pretty clearly boolean values.

Copy link
Collaborator

Choose a reason for hiding this comment

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

to be a bit more clear I mean that it doesn't necessarily hurt to keep <boolean>

Copy link
Contributor

@azaleacolburn azaleacolburn left a comment

Choose a reason for hiding this comment

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

Don't forget to merge dev

Comment on lines -14 to +15
const [useGamepad, setUseGamepad] = useState<boolean>(selectedScheme.usesGamepad)
const [useTouchControls, setUseTouchControls] = useState<boolean>(selectedScheme.usesTouchControls)
const [useGamepad, setUseGamepad] = useState(selectedScheme.usesGamepad)
const [useTouchControls, setUseTouchControls] = useState(selectedScheme.usesTouchControls)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think usesGamepad and usesTouchControls are pretty clearly boolean values.

@rutmanz rutmanz requested a review from Dhruv-0-Arora July 22, 2025 20:44
Copy link
Member

@PepperLola PepperLola left a comment

Choose a reason for hiding this comment

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

LGTM

@PepperLola PepperLola merged commit 0d01de3 into dev Jul 23, 2025
16 checks passed
@PepperLola PepperLola deleted the zachr/1945/input-scheme-mode-conflict branch July 23, 2025 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui/ux Relating to user interface, or in general, user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants