Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Numerical keys should navigate launcher and field buttons, when they have focus (not a global hotkey, and no opt/alt) #63

Closed
Tracked by #44
samreid opened this issue Jan 9, 2024 · 4 comments
Assignees
Labels

Comments

@samreid
Copy link
Member

samreid commented Jan 9, 2024

No description provided.

@samreid samreid self-assigned this Jan 9, 2024
@samreid
Copy link
Member Author

samreid commented Jan 10, 2024

Current patch:

Subject: [PATCH] Move custom actions to the left and basic actions to the right, see https://github.com/phetsims/projectile-data-lab/issues/62
---
Index: js/common-vsm/view/FieldSelectorPanel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common-vsm/view/FieldSelectorPanel.ts b/js/common-vsm/view/FieldSelectorPanel.ts
--- a/js/common-vsm/view/FieldSelectorPanel.ts	(revision c2d449bc47403070b24fbc64a12969e17eed9fde)
+++ b/js/common-vsm/view/FieldSelectorPanel.ts	(date 1704904154580)
@@ -99,11 +99,9 @@
 
     // a listener that selects a field based on the keystroke, regardless of where focus is in the document
     this.addInputListener( new KeyboardListener( {
-      keys: [ 'alt+1', 'alt+2', 'alt+3', 'alt+4', 'alt+5', 'alt+6', 'alt+7', 'alt+8' ] as const,
-      global: true,
+      keys: [ '1', '2', '3', '4', '5', '6', '7', '8' ] as const,
       callback: ( event, keysPressed ) => {
-        const numberKey = keysPressed.split( '+' )[ 1 ];
-        const key = parseInt( numberKey, 10 );
+        const key = parseInt( keysPressed, 10 );
         fieldProperty.value = fields[ key - 1 ];
       }
     } ) );

This works well at runtime, but the focus region doesn't move. I'll likely need help on this from @jessegreenberg or @zepumph. Can you please advise?

Apply the patch, run PDL. Go to screen 1. Tab to the field panel. Press 6. Notice that the 6 button is pressed but the focus is still on another button:

image

@samreid samreid assigned zepumph and jessegreenberg and unassigned samreid Jan 10, 2024
@jessegreenberg
Copy link
Contributor

RectangularRadioButtonGroup has a getButtonForValue so you can focus the button like this in your KeyboardListener callback:

fieldRadioButtonGroup.getButtonForValue( fieldProperty.value ).focus();

I thought about putting a focus call RectangularRadioButtonGroup when the Property changes, but I don't think that would be a good UX if the Property changes while the user is somewhere else in the sim. Maybe that is debatable, let me know if you would like to discuss this part further!

@samreid
Copy link
Member Author

samreid commented Jan 17, 2024

Thanks @jessegreenberg, that is working well! I'll commit momentarily.

@samreid
Copy link
Member Author

samreid commented Jan 17, 2024

Pushed. This was straightforward and matches the design recommendation. I don't see any odd interactions or need for code review. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants