iOS + Android: Unify onDrag snap API#196
Conversation
Resolves #165 Update PROPS Readme Update AlertAreas TestId
|
@rotemmiz this would be the first PR regarding the splitting of the huge one. Can you verify? |
|
There's still much noise in this PR, lots of auto formatting. It's very hard to track changes and review this way 😣 |
|
check. removed the noise. sorry, VSCode was messing up, even on the .md files |
|
any update @rotemmiz ? |
| state ?: [NSNull null], @"state", | ||
| deltaFromOrigin.x ?: 0, @"x", | ||
| deltaFromOrigin.y ?: 0, @"y", | ||
| targetSnapPointId ?: [NSNull null], @"targetSnapPointId", |
There was a problem hiding this comment.
So, assuming that the user hasn't provided an id, end returns null instead of an empty string ? was even able to return an empty string before or did it crash ?
There was a problem hiding this comment.
The crash was based on the circumstance that an Interactable View got no SnapPoints. iOS was crashing then (Getting -id- of not present -point- Object)
I´m glad for discussion if we should either just fix the crash and provide an empty string if no id is provided, or just adapt the "not present" value.
Also i just looked up that i didn´t check Android Start Event. There we are sending an empty string on Start, but an undefined value on end if no -id- is provided.
So we can adapt the previous behaviour and just send out on Android also the empty string
OR
Send out on Start an undefined value, on End undefined if not present or String if present on both platforms
There was a problem hiding this comment.
Most important is that we align the expected behavior on both platforms. If the Android behavior is working as expected let's just copy it on iOS, empty string is falsy in JS anyway.
|
Okay, i just pushed the changes. The Issue #165 was also appearing on android. |
|
@rotemmiz just a ping, sorry for disturbing you. want to proceed, but the next PR`s would be based on unify and the detox test is based on the snapPoint fix |
|
resolved merge conflicts @rotemmiz |
| </View> | ||
| <Interactable.View | ||
| snapPoints={[{x: -140, y: -250}, {x: 140, y: -250}, {x: -140, y: 250}, {x: 140, y: 250}]} | ||
| snapPoints={[{x: -140, y: -250, id: 'testId'}, {x: 140, y: -250}, {x: -140, y: 250}, {x: 140, y: 250}]} |
There was a problem hiding this comment.
This project is really hard to test , that's one of the reasons we are having hard time with maintenance.
How did you test that id really works as expected on both platforms?
Kinda unrelated to this PR:
We may need to define some sort of way to test the API. Maybe with a special test app (in this repo) that asserts on the API behavior in runtime. We can drive it using Detox. It won't be able to "feel" the correctness of the physics, but at least it will have some sort of coverage.
There was a problem hiding this comment.
Hi, yap, i experienced the same. Testing the results is really depending on the feeling of the interaction. But at least the callbacks are testable pretty good. I began with testing (merged manually this fix in the detox test branch) and also with the travis testing already.
For this purpose i enhanced the playground views. So in the onDrag example the following situations are tested:
- TestId provided and visible
- TestId not provided and element not visible
- No SnapPoints provided and no crash
You can see the basic test implementations here:
https://github.com/mkuczera/react-native-interactable/commits/implement-tests
As i said: I would begin with the callbacks to be testable. Beside of that i´m currently setting up the travis enviroment also for android. For iOS it´s ready.
|
hey, what´s the current status @rotemmiz @ButtersHub |
2 participants
Fixes Issue #165
Update PROPS Readme
Set proper intent