Replace hammerJS with native touch events #452
Conversation
| domElement: this._el.nativeElement, | ||
| onSwipeMove: event => { | ||
| if (event.direction === this.config.slidingDirection) { | ||
| this._zone.run(() => this.updateSlider({ value: event.distance, active: true })); |
There was a problem hiding this comment.
We should not use zone.run() here because we don't want to trigger so many change detections while dragging
There was a problem hiding this comment.
Ok. I'll try to avoid it
There was a problem hiding this comment.
Looks like hammerJS somehow triggers ChangeDetection on each pan event, but here I don't see any possibility to trigger async pipe outside of ngZone or without manually call detection. Here an example https://stackoverflow.com/a/35513390
Mb we can avoid ngStyles and use native js styles change, but don't think it's a good solution
There was a problem hiding this comment.
I'll continue with mouse events and thumbs swiping for now. Let me know if you have any ideas how to avoid change detection here.
There was a problem hiding this comment.
As i see we can improve performance using custom async pipe with subscription outside of ngZone as discussed in angular/angular#36139
Example: https://stackblitz.com/edit/angular-async-pipe-outside-angular?file=src%2Fapp%2Fvo-async.pipe.ts
There was a problem hiding this comment.
I replaced all async pipes with this custom pipe, and yes, it's really effective. Now we can create an observable outside of ngZone and don't care about changeDetection.
| } | ||
|
|
||
| ngOnInit() { | ||
| if (this.config.gestures && typeof Hammer !== 'undefined') { |
There was a problem hiding this comment.
We should only enable swiping if user enables the gestures in config
| } | ||
| private onSwipe(e: SwipeEvent) { | ||
| if (e.direction === this.config.slidingDirection) { | ||
| const velocity = e.distance / (e.moveEvent.sourceEvent.timeStamp - e.startEvent.sourceEvent.timeStamp); |
There was a problem hiding this comment.
Would be better if we get the velocity with the SwipeEvent
There was a problem hiding this comment.
I agree. It was here just to test :)
|
@MurhafSousli strange about swiping. I just retested it in chrome. Looks like is working https://amdaris-my.sharepoint.com/:v:/p/vladimir_petrusevici/EShrbtcG6pFDng1Z2TeSJQ8BzEmBGQIiCwkzAaM7tEAXzg?e=UBLKuq |
|
Also I see a problem with autoplay. Application will never have a stable state when autoPlay = true in the current implementation. I think this observable with delay should be also moved outside ngZone. |
Fixed with custom async pipe. Now the app is always stable. |
|
Implemented mouse events |
|
Aaaand looks like all functionalities are implemented now. |
|
@MurhafSousli should i change the docs or is it better to leave it to you? |
|
I should stop pushing... sorry) |
|
@MurhafSousli Hi, I fixed the conflicts. Can you check this PR please? |
|
Hi @MurhafSousli, |
When you add many unnecessary changes, you make it difficult for me to review! please keep the PR focused next time and don't refactor parts that are not related to the replacement of hammer.js |
Sorry, i can try to recreate the PR with only necessary changes, but don't think it will be much smaller |
|
I think HammerJS is not bad, comparing the benefits we would get from implementing our own touch-events solution does not bring any value in terms of feature or functionality, we are just adding more code which we will have to maintenance, in favor of getting rid of one dependency. I will close this for now, but will keep an eye on your solution if something changed. Thanks for the PR anyway! |
Currently I implemented swiping only for touch events (not mouse) and only for gallery-swiper component (not thumbs).
MurhafSousli Can you check please if my implementation is good enough and I can continue work in this way?
For #449