Fixed #8454 - Tooltip | Touch events support for Tooltip #14452
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
|
Thanks a lot for your contribution! But, PR does not seem to fit our code format standards. Please run the 'npm run format' command and commit the changes. |
github-actions
bot
added
the
Resolution: Needs Revision
| @@ -289,6 +300,10 @@ export class Tooltip implements AfterViewInit, OnDestroy { | |||
| return this.getOption('autoHide'); | |||
| } | |||
|
|
|||
| isMobile(): boolean { | |||
| return DomHandler.isIOS() || DomHandler.isAndroid(); | |||
There was a problem hiding this comment.
shouldn't this only check if it is isTouchDevice?
| @@ -304,6 +319,18 @@ export class Tooltip implements AfterViewInit, OnDestroy { | |||
| } | |||
| } | |||
|
|
|||
| onTouchStart(e: TouchEvent) { | |||
There was a problem hiding this comment.
| onTouchStart(e: TouchEvent) { | |
| onTouchStart(e: TouchEvent): void { |
| let target = this.getTarget(this.el.nativeElement); | ||
| target.addEventListener('focus', this.focusListener); | ||
| target.addEventListener('blur', this.blurListener); | ||
| if (this.isMobile()) { |
There was a problem hiding this comment.
There are too many nested if, which makes this code hard to read. Can we split it into smaller sub-methods?
| this.el.nativeElement.addEventListener('touchstart', this.touchStartListener); | ||
| this.el.nativeElement.addEventListener('touchend', this.touchEndListener); | ||
| } else { | ||
| if (this.getOption('tooltipEvent') === 'hover') { |
There was a problem hiding this comment.
I liked what you did with isMobile can you do same here? Like isHoverEvent or so?
| this.el.nativeElement.addEventListener('mouseenter', this.mouseEnterListener); | ||
| this.el.nativeElement.addEventListener('click', this.clickListener); | ||
| this.el.nativeElement.addEventListener('mouseleave', this.mouseLeaveListener); | ||
| } else if (this.getOption('tooltipEvent') === 'focus') { |
There was a problem hiding this comment.
I liked what you did with isMobile can you do same here? Like isFocusEvent or so?
| } | ||
| } | ||
|
|
||
| onTouchEnd(e: TouchEvent) { |
There was a problem hiding this comment.
| onTouchEnd(e: TouchEvent) { | |
| onTouchEnd(e: TouchEvent): void { |
| if (!this.container && !this.showTimeout) { | ||
| this.showTimeout = setTimeout(() => { | ||
| this.activate(); | ||
| }, 500); |
There was a problem hiding this comment.
500 was chosen based on what? Can it be less?
|
@mehmetcetin01140 Are you still interested in finalizing this pull request? I was just considering contributing this feature since i need it but it seems that you are almost done already |
|
@mehmetcetin01140 Poke? |
|
@cetincakiroglu Why was this closed, has a decision been made through another channel regarding a better approach to follow? Otherwise is it ok if I open a new PR to carry on this work? |
Fixes #8454