-
Notifications
You must be signed in to change notification settings - Fork 50
Make buttons work with keypresses #86
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,7 +49,7 @@ const DEFAULT_CPX_STATE: ICpxState = { | |
switch: false | ||
}; | ||
|
||
const SIMULATOR_BUTTO_WIDTH = 60; | ||
const SIMULATOR_BUTTON_WIDTH = 60; | ||
|
||
interface vscode { | ||
postMessage(message: any): void; | ||
|
@@ -62,7 +62,6 @@ const sendMessage = (type: string, state: any) => { | |
}; | ||
|
||
class Simulator extends React.Component<any, IState> { | ||
private keyPressed = false; | ||
constructor(props: IMyProps) { | ||
super(props); | ||
this.state = { | ||
|
@@ -75,7 +74,7 @@ class Simulator extends React.Component<any, IState> { | |
this.onMouseDown = this.onMouseDown.bind(this); | ||
this.onMouseUp = this.onMouseUp.bind(this); | ||
this.onMouseLeave = this.onMouseLeave.bind(this); | ||
this.playSimulatorClick = this.playSimulatorClick.bind(this); | ||
this.togglePlayClick = this.togglePlayClick.bind(this); | ||
this.refreshSimulatorClick = this.refreshSimulatorClick.bind(this); | ||
} | ||
|
||
|
@@ -130,53 +129,69 @@ class Simulator extends React.Component<any, IState> { | |
</div> | ||
<div className="buttons"> | ||
<Button | ||
onClick={this.playSimulatorClick} | ||
onClick={this.togglePlayClick} | ||
image={image} | ||
label="play" | ||
width={SIMULATOR_BUTTO_WIDTH} | ||
width={SIMULATOR_BUTTON_WIDTH} | ||
/> | ||
<Button | ||
onClick={this.refreshSimulatorClick} | ||
image={RefreshLogo} | ||
label="refresh" | ||
width={SIMULATOR_BUTTO_WIDTH} | ||
width={SIMULATOR_BUTTON_WIDTH} | ||
/> | ||
</div> | ||
</div> | ||
); | ||
} | ||
|
||
protected playSimulatorClick(event: React.MouseEvent<HTMLElement>) { | ||
this.setState({ ...this.state, play_button: !this.state.play_button }); | ||
protected togglePlayClick() { | ||
sendMessage("play-simulator", !this.state.play_button); | ||
this.setState({ ...this.state, play_button: !this.state.play_button }); | ||
const button = | ||
window.document.getElementById(CONSTANTS.ID_NAME.PLAY_BUTTON) || | ||
window.document.getElementById(CONSTANTS.ID_NAME.STOP_BUTTON); | ||
if (button) { | ||
button.focus(); | ||
} | ||
} | ||
|
||
protected refreshSimulatorClick(event: React.MouseEvent<HTMLElement>) { | ||
protected refreshSimulatorClick() { | ||
sendMessage("refresh-simulator", true); | ||
const button = window.document.getElementById( | ||
CONSTANTS.ID_NAME.REFRESH_BUTTON | ||
); | ||
if (button) { | ||
button.focus(); | ||
} | ||
} | ||
|
||
protected onKeyEvent(event: KeyboardEvent, active: boolean) { | ||
let button; | ||
const target = event.target as SVGElement; | ||
// Guard Clause | ||
if (target === undefined) { | ||
return; | ||
} | ||
|
||
if ([event.code, event.key].includes(CONSTANTS.KEYBOARD_KEYS.ENTER)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does changing to a switch statement make sense? |
||
if (target) { | ||
button = window.document.getElementById(target.id); | ||
if (button) { | ||
event.preventDefault(); | ||
if (button.id.includes("SWITCH")) { | ||
// Switch | ||
this.handleClick(button, active); | ||
} else if (active && !this.keyPressed) { | ||
// Send one keydown event | ||
this.handleClick(button, active); | ||
this.keyPressed = true; | ||
} else if (!active) { | ||
// Keyup event | ||
this.handleClick(button, active); | ||
this.keyPressed = false; | ||
} | ||
} | ||
} | ||
button = window.document.getElementById(target.id); | ||
} else if ([event.code, event.key].includes(CONSTANTS.KEYBOARD_KEYS.A)) { | ||
button = window.document.getElementById(CONSTANTS.ID_NAME.BUTTON_A); | ||
} else if ([event.code, event.key].includes(CONSTANTS.KEYBOARD_KEYS.B)) { | ||
button = window.document.getElementById(CONSTANTS.ID_NAME.BUTTON_B); | ||
} else if ([event.code, event.key].includes(CONSTANTS.KEYBOARD_KEYS.S)) { | ||
button = window.document.getElementById(CONSTANTS.ID_NAME.SWITCH); | ||
} else if (event.key === CONSTANTS.KEYBOARD_KEYS.CAPITAL_F) { | ||
this.togglePlayClick(); | ||
} else if (event.key === CONSTANTS.KEYBOARD_KEYS.CAPITAL_R) { | ||
this.refreshSimulatorClick(); | ||
} | ||
|
||
if (button) { | ||
event.preventDefault(); | ||
this.handleClick(button, active); | ||
button.focus(); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,8 +3,22 @@ | |
|
||
// Key events | ||
export const CONSTANTS = { | ||
ID_NAME: { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
REFRESH_BUTTON: "refresh-button", | ||
STOP_BUTTON: "stop-button", | ||
SWITCH: "SWITCH" | ||
}, | ||
KEYBOARD_KEYS: { | ||
ENTER: "Enter" | ||
A: "KeyA", | ||
B: "KeyB", | ||
CAPITAL_R: "R", | ||
CAPITAL_F: "F", | ||
ENTER: "Enter", | ||
S: "KeyS" | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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