-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Filter config #3710
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
Filter config #3710
Conversation
| if (PropsToFilter.has(key)) { | ||
| continue; | ||
| } |
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 the whitelist would be a better pick here. We could have known passable props (the configs), known unpassable props (what is currently in PropsToFilter), and we could warn about anything else.
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.
Do you mean something like
if (whitelist.has(prop)) {
...
} else if (PropsToFilter.has(prop)) {
continue;
} else {
console.warn(...)
}?
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.
Basically, yes. There should be arrays of all possible values for each handler somewhere already.
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 problem here is that we cannot simply take specific gesture properties. Let's say we pick Pan props to be in whitelist, then, for example, mouseButton is not direct Pan property, therefore it will be ignored.
On the other hand I can see why this may be useful, so we could make list of common props, then merge them with handlers' properties 😅
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 one 4ff105a seems to do the job. Let me know what you think.
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.
Actually not really, I forgot that this is function which prepares config to be passed into native side. In that case some of the props are wrongly white-listed, as Pan doesn't accept for example activeOffsetX, but rather activeOffsetXStart and activeOffsetXEnd etc.
In that case I think we will have to make those lists, since they don't exist in old types.
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 forgot that this is function which prepares config to be passed into native side.
Okay, I've fixed that... though to get rid of circular dependencies I had to split types and hooks, so it grew quite large 🙈
Let me know what you think about this whitelist approach, and if you have any thoughts/ideas how to make it look better than it is now. Also, I'm not sure about how it will work with #3731 🙈
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.
As far as I see it won't conflict with #3731. But we'll have to test to make sure.
| [SingleGestureName.Fling, withCommonProps(flingGestureHandlerProps)], | ||
| ]); | ||
|
|
||
| const EMPTY_WHILE_LIST = new Set<string>(); |
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.
Similar thing will be added in #3689, so we will have to find good place to keep 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.
Shouldn't it be EMPTY_WHITE_LIST?
j-piasecki
left a comment
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!
To be honest, I meant a single big list of props allowed for all gestures, but this is even better 😄.
| [SingleGestureName.Fling, withCommonProps(flingGestureHandlerProps)], | ||
| ]); | ||
|
|
||
| const EMPTY_WHILE_LIST = new Set<string>(); |
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.
Shouldn't it be EMPTY_WHITE_LIST?
Description
This PR adds logic that filters
configobject before passing it to native side.Test plan
Tested on the following code: