Make buttons work with keypresses #86
Conversation
LukeSlev
requested review from
abmahdy,
adclements,
markAtMicrosoft and
smmatte
as code owners
| this.setState({ ...this.state, play_button: !this.state.play_button }); | ||
| sendMessage("play-simulator", !this.state.play_button); | ||
| const button = |
There was a problem hiding this comment.
If this is a play action, would the button focus go to the stop button? The code here seems to keep focus on the play button.
There was a problem hiding this comment.
So maybe this is just poorly named. The play button is also the stop button when the code is running so this is grabbing whichever icon is available. I can rename this if that helps! @adclements
| if (target === undefined) { | ||
| return; | ||
| } | ||
| console.log("event", event); | ||
| if ([event.code, event.key].includes(CONSTANTS.KEYBOARD_KEYS.ENTER)) { |
There was a problem hiding this comment.
The if case seems to be building up. If we don't have any more sensors that require key press functionality, this should be fine?
There was a problem hiding this comment.
Any ideas on how to do this differently? I don't have any off the top of my head
There was a problem hiding this comment.
does changing to a switch statement make sense?
| BUTTON_A: "BTN_A_OUTER", | ||
| BUTTON_AB: "BTN_AB_OUTER", | ||
| BUTTON_B: "BTN_B_OUTER", | ||
| PLAY_BUTTON: "play-button", |
There was a problem hiding this comment.
Might want to keep consistent with naming convention here in the future. There appears to be uppercase with underscores and lowercase with dashes.
There was a problem hiding this comment.
Would it be possible to just change either the class name or id name since I think having these together is good. I wouldn't separate it.
|
I'm running into an issue where if I press |
Co-Authored-By: Jonathan Wang <[email protected]>
|
@jonthanwangg made changes! |
Description:
This PR lets keypresses press the buttons on the webview.
Type of change
Please delete options that are not relevant.
Testing:
Checklist: