-
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 preventClickOnDrag option to PanInput #199
Conversation
c0b70d7
to
dade936
Compare
There are a few things to check.
|
@@ -353,6 +358,9 @@ export class PanInput implements InputType { | |||
this._observer = observer; | |||
this._enabled = true; | |||
this._activeEvent = activeEvent; | |||
if (this.options.preventClickOnDrag) { | |||
this.element?.addEventListener("click", this._preventClickWhenDragged); |
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.
I think it shouldn't be activated if the element doesn't exist.
Like,
if (!activeEvent || !this.element) {
return;
}
Actually, I wonder in what case the element be null
, as it receives the element in the constructor.
Please check whether the type is incorrect for 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.
This seems to be unnecessary since existence of the element should be guaranteed by user at the timing of _attachElementEvent
, unlike _detachElementEvent
which element may not exist while input is disconnected.
I think we can throw an error if the target element does not exist at _attachElementEvent
.
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
@@ -359,14 +359,14 @@ export class PanInput implements InputType { | |||
this._enabled = true; | |||
this._activeEvent = activeEvent; | |||
if (this.options.preventClickOnDrag) { | |||
this.element?.addEventListener("click", this._preventClickWhenDragged); | |||
this.element.addEventListener("click", this._preventClickWhenDragged); |
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.
Subtract the frequently used this property with a constant.
const element = this.element;
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
const element = this.element; | ||
element?.removeEventListener("keydown", this._onKeydown, false); | ||
element?.removeEventListener("keypress", this._onKeydown, false); | ||
element?.removeEventListener("keyup", this._onKeyup, false); |
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'd rather do it like:
const element = this.element;
if (element) {
element.removeEventListener("keydown", this._onKeydown, false);
element.removeEventListener("keypress", this._onKeydown, false);
element.removeEventListener("keyup", this._onKeyup, false);
}
Because simply, the one you wrote turns like this on tsc compiler:
var element = this.element;
element === null || element === void 0 ? void 0 : element.removeEventListener("keydown", this._onKeydown, false);
element === null || element === void 0 ? void 0 : element.removeEventListener("keypress", this._onKeydown, false);
element === null || element === void 0 ? void 0 : element.removeEventListener("keyup", this._onKeyup, false);
Details
This adds the option to cancel the click event when a valid drag occurs in
PanInput
andRotatePanInput
.We currently implemented this feature on Flicking that uses
PanInput
.However, there are cases when you want to use this
preventClickOnDrag
feature in other UIs that usesPanInput
.After we add
preventClickOnDrag
toPanInputOption
, Flicking can exclude implementation aboutpreventClickOnDrag
and handle it byPanInputOption
.