-
Notifications
You must be signed in to change notification settings - Fork 54
improvement: style of pressed buttons #19
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
Conversation
src/membrane-keypad-element.ts
Outdated
} | ||
|
||
private up(key: string) { | ||
private up(key: string, event: any) { |
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.
You can either use UIEvent
or KeyboardEvent | MouseEvent | TouchEvent
(I personally prefer the first one)
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.
To use either option, I had to declare 'event.currentTarget as SVGElement' to avoid TS error: Property 'classList' does not exist on type 'EventTarget'. I updated with the first option - UIEvent.
src/membrane-keypad-element.ts
Outdated
private down(key: string, event: UIEvent) { | ||
if (!this.pressedKeys.has(key)) { | ||
const currTarget = event.currentTarget as SVGElement; | ||
currTarget.classList.add('pressed'); |
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.
What's the reason you need the pressed
class?
It seems like the :active
selector that you have above already does the trick
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.
The active selector isn't triggered when pressing the space key, so I added this class.
But it has an isssue - while pressing space, if you press tab and press space again or click with the mouse on another button, it will display multiple buttons as pressed. I can fix the tab option by removing the class when pressing tab. But I couldn't think of a way to fix the mouse selection. Do you think I should check every button on mousedown? Got a better idea?
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.
Good point!
So perhaps you could:
- For the tab issue - remove the
pressed
class onblur
event. That's way, when you press the tab key, the element loses focus, and the highlight is gone. - Make the event parameter optional and only pass it on keyboard event, so it won't get added on mouse events (this will probably solve the issue with the class sticking around when you click on the button, and then drag the mouse pointer out of the element area, which the button is still pressed)
2b34390
to
60dc651
Compare
* improvement: style of pressed buttons * improvement: buttons pressed Co-authored-by: Karin <[email protected]>
No description provided.