Skip to content
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 custom gesture support #70

Merged
merged 7 commits into from
Jun 12, 2023
Merged

feat: add custom gesture support #70

merged 7 commits into from
Jun 12, 2023

Conversation

MrSquaare
Copy link
Contributor

@MrSquaare MrSquaare commented Jun 8, 2023

This PR allows users to:

  • Customize gestures (react-native-gesture-handler only)
  • Disable gesture support

Related to #64
Documentation PR: #71

API

gesture property (only if useRNGH is true)

If the user wishes to add new gestures, modify existing ones or remove them, this property allows him to do so.

gesture can take :

  • A composed gesture
  • A gesture
  • A gesture array
  • A function that return a composed gesture, a gesture or a gesture array

The function provides defaultGestures gesture array and dispatchEvents function to send events to ZRender.

Note: If gesture is an array or returns an array, gestures will be processed by race (Gesture.Race).

handleGesture property (true by default)

If the user wishes to disable gesture support (e.g.: doesn't need it, wants to use an external gesture system), this property allows him to do so.
In addition, the reference passed to SkiaChart/SvgChart now has a dispatchEvents method to send events to ZRender.(myRef.current.dispatchEvents).

Additional info/requests/questions

  • throttle has its own file (exposed, so that users can use it if necessary)
  • the calc* functions have been moved to the PanResponderHandler file, as they are only used here
  • ref instead of state for the zrenderId (to avoid unnecessary re-rendering, should it be set to null when the component is unmounted?)
  • Why does PanGesture listen to onBegin and not onStart?
  • Why are mousedown and mousemove sent at the same time for a press? Is mouseclick not enough?
  • Shouldn't we add throttle on onUpdate of PinchGesture?

@zhiqingchen
Copy link
Member

Thanks for the contribution, I will review the code, also, the developer needs to sign the commit.

Guillaume Bonnet and others added 3 commits June 8, 2023 11:22
- disable gesture
- use custom RNGH gestures

Signed-off-by: Guillaume Bonnet <mrsquaare@mrsquaare.fr>
Signed-off-by: Guillaume Bonnet <mrsquaare@mrsquaare.fr>
Signed-off-by: Guillaume Bonnet <mrsquaare@mrsquaare.fr>
@zhiqingchen
Copy link
Member

https://github.com/wuba/taro-playground/blob/main/src/pages/explore/charts/pages/graphic/LineDraggable.tsx

This is a good test example to see how the three gestures currently available are actually used.

Additional info/requests/questions

throttle has its own file (exposed, so that users can use it if necessary)

the calc* functions have been moved to the PanResponderHandler file, as they are only used here

ref instead of state for the zrenderId (to avoid unnecessary re-rendering, should it be set to null when the component is unmounted?)

✅, No need to set to null

Why does PanGesture listen to onBegin and not onStart?

https://echarts.apache.org/examples/en/editor.html?c=line-draggable

Gesture operations should get feedback as quickly as possible, and using onStart for charts like this type can make the response feel a little slow.

Why are mousedown and mousemove sent at the same time for a press? Is mouseclick not enough?

Maybe 'click' is possible, I haven't tried it yet.

Shouldn't we add throttle on onUpdate of PinchGesture?

I think you can add low latency throttling, you've thought it through.

@zhiqingchen zhiqingchen self-requested a review June 8, 2023 09:51
@zhiqingchen
Copy link
Member

By the way, can you provide a demo of using custom gestures?

@zhiqingchen
Copy link
Member

we also need update document.

@MrSquaare
Copy link
Contributor Author

By the way, can you provide a demo of using custom gestures?

we also need update document.

Sure, I'll update this
Should I provide a new example, or would a snack be enough? (requires the package to be published, I guess)

@MrSquaare
Copy link
Contributor Author

Why are mousedown and mousemove sent at the same time for a press? Is mouseclick not enough?

Maybe 'click' is possible, I haven't tried it yet.

I made a typo, I meant "just send mousedown instead of mousedown + mousemove"
Here is a snippet of the web behavior, a click doesn't seem to send a mousemove event until the cursor has moved. There may be another reason for this, I was just wondering.

Signed-off-by: Guillaume Bonnet <mrsquaare@mrsquaare.fr>
Signed-off-by: Guillaume Bonnet <mrsquaare@mrsquaare.fr>
@MrSquaare MrSquaare mentioned this pull request Jun 8, 2023
@zhiqingchen zhiqingchen added this to the 1.2.0 milestone Jun 9, 2023
@zhiqingchen zhiqingchen linked an issue Jun 9, 2023 that may be closed by this pull request
@zhiqingchen
Copy link
Member

Why are mousedown and mousemove sent at the same time for a press? Is mouseclick not enough?

Maybe 'click' is possible, I haven't tried it yet.

I made a typo, I meant "just send mousedown instead of mousedown + mousemove" Here is a snippet of the web behavior, a click doesn't seem to send a mousemove event until the cursor has moved. There may be another reason for this, I was just wondering.

I finally found out why it was handled this way, the initial version of the code references this project https://github.com/ecomfe/echarts-for-weixin/blob/master/ec-canvas/ec-canvas.js#L201-L216 .
But I think the mousemove event, indeed, can be removed and tested with no effect.

Signed-off-by: Guillaume Bonnet <mrsquaare@mrsquaare.fr>
Signed-off-by: Guillaume Bonnet <mrsquaare@mrsquaare.fr>
@zhiqingchen zhiqingchen merged commit 01a35c3 into wuba:main Jun 12, 2023
1 check passed
@zhiqingchen
Copy link
Member

zhiqingchen commented Jun 14, 2023

Why are mousedown and mousemove sent at the same time for a press? Is mouseclick not enough?

Maybe 'click' is possible, I haven't tried it yet.

I made a typo, I meant "just send mousedown instead of mousedown + mousemove" Here is a snippet of the web behavior, a click doesn't seem to send a mousemove event until the cursor has moved. There may be another reason for this, I was just wondering.

I finally found out why it was handled this way, the initial version of the code references this project https://github.com/ecomfe/echarts-for-weixin/blob/master/ec-canvas/ec-canvas.js#L201-L216 . But I think the mousemove event, indeed, can be removed and tested with no effect.

The "mousemove" is dispatched to simulate "hover" behavior, such as highlighting the bar when touched, without requiring the user to move their finger, the previous experience was more intuitive to the user. You can see the corresponding effect in the example directory.
I need to add the code back. @MrSquaare What do you think about it?

@MrSquaare
Copy link
Contributor Author

The "mousemove" is dispatched to simulate "hover" behavior, such as highlighting the bar when touched, without requiring the user to move their finger, the previous experience was more intuitive to the user. You can see the corresponding effect in the example directory. I need to add the code back. @MrSquaare What do you think about it?

@zhiqingchen Sure, if it introduced a behavior regression, the commit should be reverted

@MrSquaare MrSquaare deleted the feature/customize-gestures branch June 14, 2023 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom(ize) gestures
2 participants