-
-
Notifications
You must be signed in to change notification settings - Fork 1k
[Web] Config refactor - update logic and drop class field #3673
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
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.
Given that in updateGestureConfig
we have:
this._config = {
enabled,
dispatchesAnimatedEvents,
...props,
};
updateGestureConfig
acts as setGestureConfig
. The point of separating this method in module was to have set
method which sets the config to new one and update
method which updates only changed fields.
Right now doing this:
console.log(this.config);
this._config = {
enabled,
dispatchesAnimatedEvents,
...props,
};
console.log(this.config);
yields this:

Okay, I had previously not understood this was the point, as your PR didn't add this, it simply renamed the function. Even before your PR checking if the fields exist was already built into |
Not really. It added |
Yeah, I could have worded that better. I meant that iOS / android implementations required fewer changes within the |
packages/react-native-gesture-handler/src/web/handlers/GestureHandler.ts
Show resolved
Hide resolved
if ( | ||
config.shouldCancelWhenOutside !== undefined && | ||
this.config.shouldCancelWhenOutside !== undefined | ||
) { |
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 ( | |
config.shouldCancelWhenOutside !== undefined && | |
this.config.shouldCancelWhenOutside !== undefined | |
) { | |
if (config.shouldCancelWhenOutside !== undefined) { |
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.
Also this check is quite weird - if config.shouldCancelWhenOutside
is defined, then it will be assigned into this.config.shouldCancelWhenOutside
in the loop above. And if config.shouldCancelWhenOutside
is undefined
, then nothing really happens.
Note, I assumed that the for loop above doesn't check for key differences.
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.
Fixed, got carried away 4fd97f8a1b8
if ( | ||
config.dispatchesAnimatedEvent !== undefined && | ||
this.config.dispatchesAnimatedEvents !== undefined | ||
) { |
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 ( | |
config.dispatchesAnimatedEvent !== undefined && | |
this.config.dispatchesAnimatedEvents !== undefined | |
) { | |
if (config.dispatchesAnimatedEvent !== undefined) { |
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.
Same as above
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.
Checking undefined was just to make eslint happy, fixed in 4fd97f8a1b8
for (const key of Object.keys(config)) { | ||
if (this.config[key] !== config[key]) { | ||
this.config[key] = config[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.
for (const key of Object.keys(config)) { | |
if (this.config[key] !== config[key]) { | |
this.config[key] = config[key]; | |
} | |
} | |
for (const key of Object.keys(config)) { | |
this.config[key] = config[key]; | |
} |
I don't think we need this check, I'd just assign new values.
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 agree, fixed 4fd97f8a1b8
packages/react-native-gesture-handler/src/web/handlers/GestureHandler.ts
Show resolved
Hide resolved
SharedValues
in handler configThere 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.
Took a quick look, and the PR generally looks good. Can you please merge the next
branch so that only actually modified files are shown in the diff?
} | ||
|
||
if (config.dispatchesAnimatedEvents !== undefined) { | ||
this.forAnimated = config.dispatchesAnimatedEvents; |
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.forAnimated = config.dispatchesAnimatedEvents; | |
this.dispatchesAnimatedEvents = config.dispatchesAnimatedEvents; |
Can we align those names?
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.
Done in 2da4640
if (config.activeOffsetXStart !== undefined) { | ||
this.activeOffsetXStart = config.activeOffsetXStart; | ||
this.hasCustomActivationCriteria = true; | ||
} | ||
|
||
if (this.config.activeOffsetXStart !== undefined) { | ||
this.activeOffsetXStart = this.config.activeOffsetXStart; | ||
|
||
if (this.config.activeOffsetXEnd === undefined) { | ||
this.activeOffsetXEnd = Number.MAX_SAFE_INTEGER; | ||
} | ||
if (config.activeOffsetXEnd !== undefined) { | ||
this.activeOffsetXEnd = config.activeOffsetXEnd; | ||
this.hasCustomActivationCriteria = true; | ||
} | ||
|
||
if (this.config.activeOffsetXEnd !== undefined) { | ||
this.activeOffsetXEnd = this.config.activeOffsetXEnd; | ||
|
||
if (this.config.activeOffsetXStart === undefined) { | ||
this.activeOffsetXStart = Number.MIN_SAFE_INTEGER; | ||
} | ||
if ( | ||
this.activeOffsetXStart !== undefined && | ||
this.activeOffsetXEnd === undefined | ||
) { | ||
this.activeOffsetXEnd = Number.MAX_SAFE_INTEGER; | ||
} else if ( | ||
this.activeOffsetXStart === undefined && | ||
this.activeOffsetXEnd !== undefined | ||
) { | ||
this.activeOffsetXStart = Number.MIN_SAFE_INTEGER; | ||
} |
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.
Is the logic with Number.MAX_SAFE_INTEGER
needed? Wouldn't that be handled by resetConfig
?
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.
Indeed, it is handled by resetConfig
. Thanks, removed in f490152.
if (config.failOffsetXStart !== undefined) { | ||
this.failOffsetXStart = config.failOffsetXStart; | ||
this.hasCustomActivationCriteria = true; | ||
} | ||
|
||
if (this.config.failOffsetXEnd !== undefined) { | ||
this.failOffsetXEnd = this.config.failOffsetXEnd; | ||
|
||
if (this.config.failOffsetXStart === undefined) { | ||
this.failOffsetXStart = Number.MIN_SAFE_INTEGER; | ||
} | ||
if (config.failOffsetXEnd !== undefined) { | ||
this.failOffsetXEnd = config.failOffsetXEnd; | ||
this.hasCustomActivationCriteria = true; | ||
} | ||
|
||
if (this.config.activeOffsetYStart !== undefined) { | ||
this.activeOffsetYStart = this.config.activeOffsetYStart; | ||
|
||
if (this.config.activeOffsetYEnd === undefined) { | ||
this.activeOffsetYEnd = Number.MAX_SAFE_INTEGER; | ||
} | ||
if ( | ||
this.failOffsetXStart !== undefined && | ||
this.failOffsetXEnd === undefined | ||
) { | ||
this.failOffsetXEnd = Number.MAX_SAFE_INTEGER; | ||
} else if ( | ||
this.failOffsetXStart === undefined && | ||
this.failOffsetXEnd !== undefined | ||
) { | ||
this.failOffsetXStart = Number.MIN_SAFE_INTEGER; | ||
} |
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.
Same
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.
Removed in f490152.
if (config.activeOffsetYStart !== undefined) { | ||
this.activeOffsetYStart = config.activeOffsetYStart; | ||
this.hasCustomActivationCriteria = true; | ||
} | ||
|
||
if (this.config.activeOffsetYStart === undefined) { | ||
this.activeOffsetYStart = Number.MIN_SAFE_INTEGER; | ||
} | ||
if (config.activeOffsetYEnd !== undefined) { | ||
this.activeOffsetYEnd = config.activeOffsetYEnd; | ||
this.hasCustomActivationCriteria = true; | ||
} | ||
|
||
if (this.config.failOffsetYStart !== undefined) { | ||
this.failOffsetYStart = this.config.failOffsetYStart; | ||
if ( | ||
this.activeOffsetYStart !== undefined && | ||
this.activeOffsetYEnd === undefined | ||
) { | ||
this.activeOffsetYEnd = Number.MAX_SAFE_INTEGER; | ||
} else if ( | ||
this.activeOffsetYStart === undefined && | ||
this.activeOffsetYEnd !== undefined | ||
) { | ||
this.activeOffsetYStart = Number.MIN_SAFE_INTEGER; | ||
} |
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.
Same
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.
Removed in f490152.
if (config.failOffsetYStart !== undefined) { | ||
this.failOffsetYStart = config.failOffsetYStart; | ||
this.hasCustomActivationCriteria = true; | ||
} | ||
|
||
if (this.config.failOffsetYEnd !== undefined) { | ||
this.failOffsetYEnd = this.config.failOffsetYEnd; | ||
if (config.failOffsetYEnd !== undefined) { | ||
this.failOffsetYEnd = config.failOffsetYEnd; | ||
this.hasCustomActivationCriteria = true; | ||
} | ||
|
||
if (this.config.failOffsetYStart === undefined) { | ||
this.failOffsetYStart = Number.MIN_SAFE_INTEGER; | ||
} | ||
if ( | ||
this.failOffsetYStart !== undefined && | ||
this.failOffsetYEnd === undefined | ||
) { | ||
this.failOffsetYEnd = Number.MAX_SAFE_INTEGER; | ||
} else if ( | ||
this.failOffsetYStart === undefined && | ||
this.failOffsetYEnd !== undefined | ||
) { | ||
this.failOffsetYStart = Number.MIN_SAFE_INTEGER; | ||
} |
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.
Same
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.
Removed in f490152.
packages/react-native-gesture-handler/src/web/handlers/PanGestureHandler.ts
Show resolved
Hide resolved
@@ -156,7 +153,7 @@ export class GestureHandlerWebDelegate | |||
} | |||
|
|||
private setUserSelect(isHandlerEnabled: boolean) { |
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.
It is probably out of scope of this PR, but if we have access to this.gestureHandler
we could do this.gestureHandler.enabled
instead of passing it as argument.
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 If the delegate is already being refactored we might as well add it.
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.
Added in 847f3ed
Co-authored-by: Michał Bert <[email protected]>
This reverts commit c78bb91.
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
public get enableContextMenu() { | ||
return this._enableContextMenu; | ||
} | ||
protected set enableContextMenu(value) { |
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.
Should they be protected
? If they're not used outside of this class we could assign directly to backing field and remove setters.
However this approach is also ok, so feel free to choose the one you prefer.
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.
Sure, I removed setters that are not used outside of the class, there is no reason to bloat the code e87ea11
Description
This PR changes how config is handled on web, following #3658 it separates update and set functionality, which allows passing only relevant, changed fields to update, as opposed to the entire config. Moreover it removes the config field from
GestureHandler
- adds necessary fields as class fields, this mimics how config is handled on iOS / Android and simplifies the code (previously some fields were both both config and class fields).Test plan
The update functionality was tested on the following code.