Popover hover close fix #1884
Popover hover close fix #1884
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1884 +/- ##
======================================
Coverage 100% 100%
======================================
Files 415 415
Lines 8746 8750 +4
Branches 1292 1292
======================================
+ Hits 8746 8750 +4
Continue to review full report at Codecov.
|
| @@ -139,7 +139,7 @@ export class SkyPopoverDirective implements OnChanges, OnDestroy { | |||
| this.closePopover(); | |||
| } | |||
| } | |||
| }); | |||
| }, 200); | |||
There was a problem hiding this comment.
I would avoid "guessing" which value works here (some browsers might take longer than others to process events), and try something more predictable. This worked for me locally:
import 'rxjs/add/operator/take';
Observable
.fromEvent(element, 'mouseleave')
.takeUntil(this.idled)
.subscribe((event: MouseEvent) => {
this.skyPopover.isMouseEnter = false;
if (this.skyPopoverTrigger === 'mouseenter') {
event.preventDefault();
if (this.skyPopover.isOpen) {
this.closePopover();
} else {
// If the mouse leaves before the popover is open,
// wait for the transition to complete before closing it.
this.skyPopover.popoverOpened.take(1).subscribe(() => {
this.closePopover();
});
}
}
});
Blackbaud-AlexKingman
assigned Blackbaud-TrevorBurch and unassigned Blackbaud-AlexKingman
Blackbaud-TrevorBurch
assigned Blackbaud-SteveBrush and unassigned Blackbaud-TrevorBurch
| this.windowRef.getWindow().setTimeout(() => { | ||
| if (this.isPopoverOpen()) { | ||
| if (this.skyPopover.isMouseEnter) { | ||
| this.skyPopover.markForCloseOnMouseLeave(); |
There was a problem hiding this comment.
@Blackbaud-TrevorBurch The popover's markForCloseOnMouseLeave() method is no longer being used, but we can't remove it since it would be a breaking change. Do you mind adding a // TODO: ... comment above the method so we don't forget to remove it when we make breaking changes?
There was a problem hiding this comment.
Sure! Done!
Blackbaud-TrevorBurch
added
Status: Ready to merge
and removed
Status: Ready for review
labels
| // Give the popover a chance to set its isMouseEnter flag before checking to see | ||
| // if it should be closed. | ||
| this.windowRef.getWindow().setTimeout(() => { | ||
| this.closePopoeverOrMarkForClose(); |
There was a problem hiding this comment.
The method is misspelled :(
There was a problem hiding this comment.
Fixed.... sorry about that.
#1816
I tested multiple intervals and found that 200ms was the minimum that could be used to ensure closure in all cases.