feat(carousel): add event on carousel slide with direction info #1406

giampierobono · 2017-03-25T16:40:14Z

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.

giampierobono · 2017-03-29T20:39:02Z

Don't understand why the build fails...in local it works

wesleycho · 2017-04-08T13:55:08Z

src/carousel/carousel.ts

+ */
+export enum NgbSlideCarouselEventDirection {
+  LEFT = <any>'left',
+  RIGHT = <any>'right'


Why type these to any and not make use of the implicit string typing?

As far as I know, you cannot assign directly a string to an enum value. In fact if I remove , typescript says: type string is no assignable to type NgbSlideCarouselEventDirection.
Is there another way to do it?

Looks like you should just be able to do

export enum NgbSlideCarouselEventDirection { left, right }

and use it like NgbSlideCarouselEventDirection.left to represent the string value left

but doing like this, NgbSlideCarouselEventDirection.left and NgbSlideCarouselEventDirection.right values won't be 0 and 1 instead of 'left' and 'right'?

What about using an union type on the variable ('left' | 'right') instead of an enum ?

I think it would be easier to use as an enum instead of strings. I was imaging using it from outside and doing something:
event.direction === NgbSlideCarouselEventDirection.LEFT
instead of
event.direction === 'left'

wesleycho · 2017-04-08T13:57:03Z

The build failure is some flakiness with Edge testing in Sauce Labs - that is to be looked into.

This PR looks mostly good to me.

wesleycho · 2017-04-10T05:39:27Z

src/carousel/carousel.ts

+        this.onCarouselSlide.emit({
+          type: 'slide',
+          prevSlideId: this.activeId,
+          newSlideId: selectedSlide.id,


Maybe these can be shortened to prev and current.

Totally agree. I will change them. Thanks for the suggestion :)

mdoing · 2017-04-12T19:09:21Z

src/carousel/carousel.ts

+    const currentActiveSlideIdx = this._getSlideIdxById(currentActiveSlideId);
+    const nextActiveSlideIdx = this._getSlideIdxById(nextActiveSlideId);
+
+    return currentActiveSlideIdx > nextActiveSlideIdx ? NgbSlideCarouselEventDirection.RIGHT :


I would expect that when you are on the first slide and click the back arrow that the direction would be RIGHT. However, when you are on the first slide with this logic it will always return LEFT, regardless of which direction you are moving in. The same thing applies if you are on the last slide, the direction will always be RIGHT, regardless of which direction you move in.

You are totally right! Thanks a lot!

I have just updated my PR. Now the logic is simpler, basically when you click on prev is always RIGHT and when you click on next is always LEFT. The computation based on the index is now done only clicking on the indicators.

pkozlowski-opensource · 2017-04-22T14:16:18Z

src/carousel/carousel.ts

@@ -43,7 +45,7 @@ export class NgbSlide {
  },
  template: `
    <ol class="carousel-indicators">
-      <li *ngFor="let slide of slides" [id]="slide.id" [class.active]="slide.id === activeId" (click)="cycleToSelected(slide.id)"></li>
+      <li *ngFor="let slide of slides" [id]="slide.id" [class.active]="slide.id === activeId" (click)="select(slide.id)"></li>


This changes the behaviour in subtle way: now we would reset timer on click. Was it intentional? If so we should add a test for it.

no it comes from the rebase. I am sorry, I will revert it. Let me know if you want to revert the package json as well and I will update my PR. Thanks for the review

pkozlowski-opensource · 2017-04-22T14:18:31Z

src/carousel/carousel.ts

+   * A carousel slide event fired when the slide transition is completed.
+   * See NgbSlideCarouselEvent for payload details
+   */
+  @Output() onCarouselSlide = new EventEmitter<NgbSlideCarouselEvent>();


can we name it just slide? We are in the carousel context so no need to add the Carousel in the name. Similarly we know that this is an event so no need for on (we are not adding it for other directives / events).

pkozlowski-opensource · 2017-04-22T14:19:18Z

package.json

    "@angular/common": "2.3.1",
+    "@angular/core": "2.3.1",


Why this change? While it doesn't hurt it always best idea to avoid unrelated changes in a single PR.

Sorry, this has been moved automatically by yarn when I did the update. It was not in alphabetical order. I can revert it if you prefer, no problem.

pkozlowski-opensource · 2017-04-22T14:20:19Z

demo/src/app/components/carousel/carousel.component.ts

@@ -8,6 +8,7 @@ import {DEMO_SNIPPETS} from './demos';
      <ngbd-api-docs directive="NgbCarousel"></ngbd-api-docs>
      <ngbd-api-docs directive="NgbSlide"></ngbd-api-docs>
      <ngbd-api-docs-config type="NgbCarouselConfig"></ngbd-api-docs-config>
+      <ngbd-api-docs-class type="NgbSlideCarouselEvent"></ngbd-api-docs-class>


I don't think this new functionality warrants a separate demo - 2 carousel side by side already look wired on our demo page so I would prefer adding more demos unless those really add a lot of value.

Isn't it the way to update the documentation adding specific information about this new event?

Documentation is generated automatically from the source code - as long as you've got JS doc on the @Output it will make it to the demo site.

I tried to remove this but then in the demo I don't see any doc related to the NgbSlideCarouselEvent, am I doing something wrong?
I am talking about this:

pkozlowski-opensource · 2017-04-22T14:21:19Z

@giampierobono thnx, it looks good generally speaking but I've left some comments that I would love to see addressed before we merge this one. Thnx again!

pkozlowski-opensource · 2017-05-02T09:11:54Z

@giampierobono are you planning to do further work on this PR? If so could you please remove the demo part and address all the remaining comments so I could merge this PR? Thnx!

giampierobono · 2017-05-02T11:38:29Z

@pkozlowski-opensource I have just reverted the demo changes and I have already applied all the other changes in comments. I was waiting for your reply to my comment about the documentation.

The PR should be ok now. Let me know if something is still missing.

Thanks again.

pkozlowski-opensource · 2017-05-02T13:12:32Z

src/carousel/carousel.ts

+  /**
+   * Event type
+   */
+  type: 'slide';


Why do we need type? Do we have other types and we need to distinguish between those? Isn't interface enough?

When I have done this the first time, I have checked the bootstrap carousel doc and there are 2 events: slide and slid (the first is for transition start and the second for transition end if I remember correctly) and the initial plan was to reproduce the same behavior, that's why we have type.
Need to add the slid event but I had not time till now (don't even know if it's really needed), so I have pushed this. I can remove it if you want, I agree that since by now there is only one event it's not necessary.

Yeh, let's remove it. We should rather have 2 different event interfaces rather than trying to discriminate those by a string.

of course 2 different events and not only one discriminated by strings :) it's the bs event structure having the 'type' prop so I have reproduced it. I am going to update the PR

pkozlowski-opensource · 2017-05-11T09:54:54Z

@giampierobono I was about to merge this PR but then I had a closer look and started to ask myself the following question: what is the use-case for exposing slide direction in the new event? Do you have a specific need? AFAIK it wasn't explicitly asked for in any of the prior issues (#1331, #1356).

If you don't have specific use-case I would prefer that we leave off this functionality as it adds a bit of code and currently people can't even visually see slide direction as we don't support animations (!)

giampierobono · 2017-05-11T12:26:06Z

@pkozlowski-opensource sorry I have just seen your message.

For me one use-case is keep in sync the carousel and some panels below it (with text adding more details to each carousel image), that will be displayed or not, following the event direction without relying only on the image id. Or another use case could be bind to the event and perform an action only if the direction is "right" or "left" and not both.

pkozlowski-opensource · 2017-05-11T13:19:32Z

@giampierobono sure, I can also imagine use cases - I was rather asking if you, or any users that you know, already has a specific use-case :-)

giampierobono · 2017-05-11T15:01:19Z

Ah ok :) For something I am working on, the second use-case is exactly what I would need...would be nice to have it.

kesarion · 2017-06-21T09:25:33Z

Is something still blocking this pull from being merged?

jspizziri · 2017-06-27T13:53:18Z

@pkozlowski-opensource,

Is there further work on this PR before it get's merged in? I have a couple of use-cases that this would be greatly helpful for.

Thanks!

giampierobono force-pushed the feature/event-on-carusel-change branch 3 times, most recently from b73773e to 11a2818 Compare March 26, 2017 12:32

pkozlowski-opensource added the component: carousel label Mar 27, 2017

giampierobono force-pushed the feature/event-on-carusel-change branch 3 times, most recently from a654011 to 933d037 Compare March 28, 2017 20:15

wesleycho reviewed Apr 8, 2017

View reviewed changes

wesleycho added the needs: review label Apr 8, 2017

wesleycho reviewed Apr 10, 2017

View reviewed changes

giampierobono force-pushed the feature/event-on-carusel-change branch from 933d037 to 3973084 Compare April 10, 2017 05:58

mdoing reviewed Apr 12, 2017

View reviewed changes

giampierobono force-pushed the feature/event-on-carusel-change branch from 3973084 to a483b45 Compare April 13, 2017 07:20

pkozlowski-opensource reviewed Apr 22, 2017

View reviewed changes

pkozlowski-opensource added needs: work and removed needs: review labels Apr 22, 2017

pkozlowski-opensource added this to the 1.0.0-alpha.25 milestone Apr 23, 2017

giampierobono force-pushed the feature/event-on-carusel-change branch 2 times, most recently from cdbee4d to 940b069 Compare April 23, 2017 20:46

giampierobono force-pushed the feature/event-on-carusel-change branch from 940b069 to 3fd6a6d Compare May 2, 2017 11:33

giampierobono force-pushed the feature/event-on-carusel-change branch from 3fd6a6d to 305091e Compare May 2, 2017 13:05

pkozlowski-opensource reviewed May 2, 2017

View reviewed changes

giampierobono force-pushed the feature/event-on-carusel-change branch from 305091e to b9d8374 Compare May 2, 2017 14:12

feat(carousel): add event on carousel slide with direction info

8422200

giampierobono force-pushed the feature/event-on-carusel-change branch from b9d8374 to 8422200 Compare May 2, 2017 14:24

pkozlowski-opensource modified the milestones: 1.0.0-alpha.26, 1.0.0-alpha.27 May 16, 2017

jspizziri mentioned this pull request May 26, 2017

[Feature Request] Customize content of Carousel Indicators #1587

Closed

pkozlowski-opensource closed this in 5d79c00 Jun 28, 2017

pkozlowski-opensource mentioned this pull request May 24, 2018

carousel expose an event of index change #1331

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(carousel): add event on carousel slide with direction info #1406

feat(carousel): add event on carousel slide with direction info #1406

giampierobono commented Mar 25, 2017 •

edited

giampierobono commented Mar 29, 2017

wesleycho Apr 8, 2017

giampierobono Apr 10, 2017

wesleycho Apr 10, 2017

giampierobono Apr 10, 2017 •

edited

thbt Apr 24, 2017 •

edited

giampierobono Apr 24, 2017

wesleycho commented Apr 8, 2017

wesleycho Apr 10, 2017

giampierobono Apr 10, 2017

mdoing Apr 12, 2017

giampierobono Apr 13, 2017

pkozlowski-opensource Apr 22, 2017

giampierobono Apr 23, 2017

pkozlowski-opensource Apr 22, 2017

pkozlowski-opensource Apr 22, 2017

giampierobono Apr 22, 2017

pkozlowski-opensource Apr 22, 2017

giampierobono Apr 22, 2017

pkozlowski-opensource Apr 22, 2017

giampierobono Apr 23, 2017

pkozlowski-opensource commented Apr 22, 2017

pkozlowski-opensource commented May 2, 2017

giampierobono commented May 2, 2017

pkozlowski-opensource May 2, 2017

giampierobono May 2, 2017 •

edited

pkozlowski-opensource May 2, 2017

giampierobono May 2, 2017

pkozlowski-opensource commented May 11, 2017

giampierobono commented May 11, 2017

pkozlowski-opensource commented May 11, 2017

giampierobono commented May 11, 2017 •

edited

kesarion commented Jun 21, 2017

jspizziri commented Jun 27, 2017

feat(carousel): add event on carousel slide with direction info #1406

feat(carousel): add event on carousel slide with direction info #1406

Conversation

giampierobono commented Mar 25, 2017 • edited

giampierobono commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giampierobono Apr 10, 2017 • edited

Choose a reason for hiding this comment

thbt Apr 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesleycho commented Apr 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkozlowski-opensource commented Apr 22, 2017

pkozlowski-opensource commented May 2, 2017

giampierobono commented May 2, 2017

Choose a reason for hiding this comment

giampierobono May 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkozlowski-opensource commented May 11, 2017

giampierobono commented May 11, 2017

pkozlowski-opensource commented May 11, 2017

giampierobono commented May 11, 2017 • edited

kesarion commented Jun 21, 2017

jspizziri commented Jun 27, 2017

giampierobono commented Mar 25, 2017 •

edited

giampierobono Apr 10, 2017 •

edited

thbt Apr 24, 2017 •

edited

giampierobono May 2, 2017 •

edited

giampierobono commented May 11, 2017 •

edited