feat(carousel): stay paused after calling `pause()` until `cycle()` is called #3225

divdavem · 2019-06-07T10:23:54Z

After setting the carousel on pause through its pause() method and clicking on navigation arrows, the carousel no longer automatically restarts. Calling the cycle() method is needed to undo the effect of the pause() method.
When pauseOnHover is true, hovering on the carousel no longer uses the pause() and cycle() methods. The hover status is stored separately, so that calling pause() and removing the mouse from the carousel no longer restarts it automatically.

The slide event now has a "source" property allowing to know what triggered the event: "timer", "arrowLeft", "arrowRight" or "indicator".
The "pause" boolean status is now also included in the slide event.

Closes #3188

Before submitting a pull request, please make sure you have at least performed the following:

read and followed the CONTRIBUTING.md guide.
built and tested the changes locally.
added/updated any applicable tests.
added/updated any applicable API documentation.
added/updated any applicable demos.

codecov-io · 2019-06-07T13:04:01Z

Codecov Report

Merging #3225 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3225      +/-   ##
==========================================
+ Coverage   90.14%   90.22%   +0.07%     
==========================================
  Files          91       91              
  Lines        2700     2720      +20     
  Branches      503      504       +1     
==========================================
+ Hits         2434     2454      +20     
  Misses        207      207              
  Partials       59       59

Flag	Coverage Δ
#e2e	`54.88% <25.64%> (-0.15%)`	⬇️
#unit	`87.37% <100%> (+0.09%)`	⬆️

Impacted Files	Coverage Δ
src/index.ts	`66.66% <ø> (ø)`	⬆️
src/carousel/carousel.module.ts	`50% <ø> (ø)`	⬆️
src/carousel/carousel.ts	`98.83% <100%> (+0.35%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a20df01...9bacd7c. Read the comment docs.

ymeine

This looks very good to me! Just a small advised change regarding one aspect of the API :)

ymeine · 2019-06-17T15:47:06Z

src/carousel/carousel.ts

+  /**
+   * Whether the pause() method was called (and no cycle() call was done afterwards).
+   */
+  pause: boolean;


I would rename this to paused, since this described the current state of the carousel, not an action linked to the event.

As @ymeine said. 👍 for renaming to paused

benouat

LGTM overall. I left out some comments to be taken care of. @divdavem could you have a look please?

benouat · 2019-06-24T07:41:45Z

src/carousel/carousel.ts

@@ -147,18 +165,39 @@ export class NgbCarousel implements AfterContentChecked,
    this.showNavigationIndicators = config.showNavigationIndicators;
  }

+  @HostListener('mouseenter')


Could you please move back these 2 @HostListener to the decorator's metadata ? When there is only one statement in the function, we usually put them directly up there.
→ '(mouseenter)': '_mouseHover$.next(true)'

benouat · 2019-06-24T07:41:59Z

src/carousel/carousel.ts

+    this._mouseHover$.next(true);
+  }
+
+  @HostListener('mouseleave')


→ '(mouseleave)': '_mouseHover$.next(false)'

benouat · 2019-06-24T07:42:33Z

src/carousel/carousel.ts

+  /**
+   * Whether the pause() method was called (and no cycle() call was done afterwards).
+   */
+  pause: boolean;


As @ymeine said. 👍 for renaming to paused

benouat · 2019-06-24T07:43:44Z

src/carousel/carousel.ts

@@ -277,4 +324,11 @@ export enum NgbSlideEventDirection {
  RIGHT = <any>'right'
 }

+export enum NgbSlideEventSource {
+  TIMER = 'timer',


Do we need to add <any> in here ? Such as in NgbSlideEventDirection

benouat · 2019-06-24T07:49:54Z

src/carousel/carousel.ts

+   */
+  pause: boolean;
+
+  /** Source triggering the slide change event */


I would make the comment a bit more explicative. It's our only way to document NgbSlideEventSource as we don't take into account enum in the doc generator.

divdavem · 2019-06-24T08:51:07Z

@ymeine and @benouat Thank you for your review.

I have renamed the pause property to paused in NgbSlideEvent.

I have also added the list of possible values in the documentation for the source property (as it is done for the direction property).

I did not move the content of the mouseEnter and mouseLeave functions to the host property because it would not compile properly in AOT mode because _mouseHover$ is private.

I did not add <any> in the NgbSlideEventSource enum because I don't know why it was done for the other enum in the file. For consistency, as you suggested @benouat, it would probably be better to remove <any> from the NgbSlideEventDirection enum in another PR.

…lled After setting the carousel on pause through its pause() method and clicking on navigation arrows, the carousel no longer automatically restarts. Calling the cycle() method is needed to undo the effect of the pause() method. When pauseOnHover is true, hovering on the carousel no longer uses the pause() and cycle() methods. The hover status is stored separately, so that calling pause() and removing the mouse from the carousel no longer restarts it automatically. The slide event now has a "source" property allowing to know what triggered the event: "timer", "arrowLeft", "arrowRight" or "indicator". The "paused" boolean status is now also included in the slide event. Closes ng-bootstrap#3188

divdavem force-pushed the carouselRefactor branch 2 times, most recently from 0a6dd45 to 09f9eba Compare June 7, 2019 12:47

benouat added component: carousel needs: review labels Jun 7, 2019

divdavem force-pushed the carouselRefactor branch from 09f9eba to 3d234c3 Compare June 7, 2019 15:47

ymeine suggested changes Jun 17, 2019

View reviewed changes

benouat requested changes Jun 24, 2019

View reviewed changes

benouat added needs: work and removed needs: review labels Jun 24, 2019

benouat added this to the 5.0 milestone Jun 24, 2019

divdavem force-pushed the carouselRefactor branch 2 times, most recently from 839cc5d to 4521742 Compare June 24, 2019 08:49

divdavem force-pushed the carouselRefactor branch 4 times, most recently from 4452f45 to 8a9e1bf Compare June 25, 2019 14:00

benouat modified the milestones: 5.0, 5.1.0 Jun 28, 2019

divdavem force-pushed the carouselRefactor branch 2 times, most recently from cae2ceb to 0aa46c9 Compare July 2, 2019 15:01

divdavem force-pushed the carouselRefactor branch from 0aa46c9 to beebdd3 Compare July 10, 2019 08:40

divdavem changed the title ~~fix(carousel): stay paused after calling pause() until cycle() is called~~ feat(carousel): stay paused after calling pause() until cycle() is called Jul 10, 2019

divdavem force-pushed the carouselRefactor branch from beebdd3 to f1a19f1 Compare July 10, 2019 13:51

benouat approved these changes Jul 16, 2019

View reviewed changes

benouat merged commit 2602154 into ng-bootstrap:master Jul 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(carousel): stay paused after calling `pause()` until `cycle()` is called #3225

feat(carousel): stay paused after calling `pause()` until `cycle()` is called #3225

divdavem commented Jun 7, 2019

codecov-io commented Jun 7, 2019 •

edited

ymeine left a comment

ymeine Jun 17, 2019

benouat Jun 24, 2019

benouat left a comment

benouat Jun 24, 2019

benouat Jun 24, 2019

benouat Jun 24, 2019

benouat Jun 24, 2019

benouat Jun 24, 2019

divdavem commented Jun 24, 2019

feat(carousel): stay paused after calling pause() until cycle() is called #3225

feat(carousel): stay paused after calling pause() until cycle() is called #3225

Conversation

divdavem commented Jun 7, 2019

codecov-io commented Jun 7, 2019 • edited

Codecov Report

ymeine left a comment

Choose a reason for hiding this comment

ymeine Jun 17, 2019

Choose a reason for hiding this comment

benouat Jun 24, 2019

Choose a reason for hiding this comment

benouat left a comment

Choose a reason for hiding this comment

benouat Jun 24, 2019

Choose a reason for hiding this comment

benouat Jun 24, 2019

Choose a reason for hiding this comment

benouat Jun 24, 2019

Choose a reason for hiding this comment

benouat Jun 24, 2019

Choose a reason for hiding this comment

benouat Jun 24, 2019

Choose a reason for hiding this comment

divdavem commented Jun 24, 2019

feat(carousel): stay paused after calling `pause()` until `cycle()` is called #3225

feat(carousel): stay paused after calling `pause()` until `cycle()` is called #3225

codecov-io commented Jun 7, 2019 •

edited