-
Notifications
You must be signed in to change notification settings - Fork 32
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
feat: add inputKey option to PanInput and WheelInput #204
Conversation
0fa4331
to
7c6d63a
Compare
@@ -130,7 +142,7 @@ export class WheelInput implements InputType { | |||
} | |||
|
|||
private _onWheel(event: WheelEvent) { | |||
if (!this._enabled) { | |||
if (!this._enabled || !isValidKey(event, this.options.inputKey)) { |
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 wheel will have to be careful.
Multi-touch in the browser works with ctrl + wheel.
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.
Since ctrl + wheel also has a zoom function in the browser, it looks like it should be handled with preventDefault
.
In Axes, preventDefault
is already applied for ctrl + wheel, can be there any additional processing needed?
* - shift: shift 키 | ||
* - ctrl: ctrl 키 | ||
* - alt: alt 키 | ||
* - meta: meta 키 </ko> |
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 action of inputKey is the same as or
.
If so, does it also support and
behavior?
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.
Currently this does not support and
behavior. I designed this as or
for the following reasons.
- Certain input devices do not have an Alt key, so it need a replacement.
- A similar option
inputButton
is implemented asor
and both options are array, it seems better to work consistently withor
.
(!inputKey || isValidKey(event, inputKey)) && | ||
(!inputButton || this._isValidButton(this._getButton(event), inputButton)) |
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.
Isn't every event valid when there're no items in inputKey
and inputButton
in this case?
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.
Since array is type of object, an empty Array is conversed to true and !inputButton
is conversed to false.
The reason to check !inputButton
here is that inputButton
is optional like in the case of a pinch gesture where there is no inputButton
option.
But for what you said, it seemed nice to add the case where inputButton
is an empty array to the unit test, so I added it.
}); | ||
|
||
it("should not trigger event when the inputButton is empty", (done) => { | ||
// Given | ||
const hold = sinon.spy(); | ||
const change = sinon.spy(); | ||
const release = sinon.spy(); |
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.
Mixing tab & space here
packages/demo/src/pages/Options.mdx
Outdated
Allow input only when one of these keys is pressed. If the array is empty, it accepts all keys with case of no key is pressed. | ||
- shift: shift key | ||
- ctrl: ctrl key | ||
- alt: alt key | ||
- meta: meta key |
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 should change the document, as it looks like ANY
replaces this
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.
LGTM
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.
LGTM
Details
This adds
InputKey
option that accepts input only when certain keys are pressed for thePanInput
andWheelInput
.Considering that some devices don't have an Alt key and use a meta key instead, I implemented
InputKey
option to be an array of keys that allows input when any one of these is pressed.