feat(carousel): allow control mouse event #2436
Conversation
Allows the user to Enable or disable the mouse event that controls the movement of carousel fix ng-bootstrap#1163 ng-bootstrap#2422
|
Please review code and merge the changes. This will be an important feature. |
There was a problem hiding this comment.
@DheerajVislavath Thank you for your contribution!
Looking at the code to try to understand the feature, I think it should be named and handled differently.
Internally you can already pause the carrousel by specifying an interval of 0. So whenever you (mouseenter) I would save the interval value, set it to 0, and restore the saved value on (mouseleave)
Regarding the name, I would suggest @Input() pauseOnHover: boolean, with a default value of false.
Same for config.
Hope the explanations are clear so you could update the PR.
|
@benouat Thank you for your suggestions. |
You're right !! I did not see these ones. Then it should be even easier. You just need to introduce the |
Allows the user to Enable or disable the mouse event that controls the movement of carousel fix ng-bootstrap#1163 ng-bootstrap#2422
|
@benouat I implemented your suggested change. Please review and process to merge the code changes. |
There was a problem hiding this comment.
We are almost good @DheerajVislavath !
Would you mind addressing the few comments I left.
Also please make sure to rebase your branch to be fast forward.
| it('should pause / resume slide change with time passage on mouse enter / leave', fakeAsync(() => { | ||
| const html = ` | ||
| <ngb-carousel> | ||
| <ngb-carousel [pauseOnHover]='pauseOnHover'> |
There was a problem hiding this comment.
I don't see the point about adding that here. It basically duplicate with your added test.
Both 'should listen to mouse events based on pauseOnHover attribute' and 'should pause / resume slide change with time passage on mouse enter / leave' are the same.
| @@ -39,8 +39,8 @@ export class NgbSlide { | |||
| 'class': 'carousel slide', | |||
| '[style.display]': '"block"', | |||
| 'tabIndex': '0', | |||
| '(mouseenter)': 'pause()', | |||
| '(mouseleave)': 'cycle()', | |||
| '(mouseenter)': 'mouseEventPause()', | |||
There was a problem hiding this comment.
I would rename it to onMouseEnter() as for now it won't be related to pause all the time.
| '(mouseenter)': 'pause()', | ||
| '(mouseleave)': 'cycle()', | ||
| '(mouseenter)': 'mouseEventPause()', | ||
| '(mouseleave)': 'mouseEventCycle()', |
There was a problem hiding this comment.
I would rename it to onMouseLeave()
| @@ -84,6 +84,11 @@ export class NgbCarousel implements AfterContentChecked, | |||
| */ | |||
| @Input() keyboard: boolean; | |||
|
|
|||
| /** | |||
| * A flag to enable or disable mouse events | |||
There was a problem hiding this comment.
mouse events is too generic in here. we only check for mouse enter/leave
I would rephrase it to Flag to enable pause/resume on mouseover.
This commit resolves issue #1163 , which allows the user to enable or disable the mouse events. Please review the pull requests and let me know if everything is good to be merged into. Thanks