feat(carousel): add animations

[fbasso]

Cherry-picked form #2817, then simplified and adapted to be aligned with recent widget development.

To be noticed:

• The slid event has been added (when the transition finish),
• The revert can be done only if the previous slide is selected.

[benouat]

LGTM! Please have a look a the couple question I left here and there in the review. Nothing big, mostly minor changes.

[maxokorokov]

Hey, @fbasso!

Same as for the accordion - all carousel demos are crashing with this version

See other comments in the code direclty.

Cheers,
Max

[maxokorokov]

This generates the following ids: ex. ngb-slide-ngb-slide-13, why is the ngb-slide- prefix necessary?
Can't we just use slide.id?

[fbasso]

If I'm not wrong, having dom ids starting with numbers is a problem. It's just to avoid this case.

[maxokorokov]

it is already prefixed with ngb-slide-, you're prefixing it twice

[fbasso]

Yes, you're right, I answered too quickly 😟. I think I prefixed it the same way than the the indicators, but it's not needed. I'll remove it.

[fbasso]

Finally, I need to prefix it. We prefixed the id only when it's not provided, this way:

@Input() id = `ngb-slide-${nextId++}`;

See the test should mark the requested slide as active for example.

[benouat]

What about not prefixing it when provided, but making sure they do not provide a number as ID ?

[fbasso]

IMO, I think the simpler (what have done in the latest version) is to prefix it in the template, not in the generated id. It keeps the freedom to provide what you want, without having to check anything.

[fbasso]

I pushed back most of the required changes. Still working on the slidevent.

Edit: still some issues to solve from unit tests

[fbasso]

Latest changes pushed. The tests fails at the moment because of a mismatch between chrome driver (support only 85) and Chrome (officially 84 at the moment).

I integrated all the required changes, but maybe still things to discuss:

• I had to change a unit test in order to have slides id starting with letters, and not numbers. The issue comes that I need to query elements in order to initialize the active class. But:

  • Html 5/Javascript is weird on this point : numbers are valid for id attributes, but querySelector('#1') leads to #1 is not a valid selector error
  • I can't prefix ids coming from the user without breaking the compatibility (even if we already breaks the compatibility here : numbers can't be used. But there is maybe less chances to met this use case (?)). It's to be discussed, as we can break the compatibility in order to prefix the id all the time (generated or given by the user): in this case, there will be no more javascript errors, but users would not be able to query the dom with the ids they gave.

• (slid) has been added at slide level. NgbSingleSlideEvent is used for the payload. I'm not sure about the naming used.

[maxokorokov]

Hey, @fbasso!

LGTM, we just need to finish this id story :)

The way it is implemented now is a breaking change, so we should either fix it or make a proper commit comment.

if you don't want to use document.getElementById(), what we should do is the following to avoid using DOM queries altogether ourselves:

<div class="carousel-inner">
      <div #slideElements *ngFor="let slide of slides; ..." [id]="slide.id" ...>
        ...
      </div>
</div>

@ViewChildren('slideElements') private _slideElements: QueryList<ElementRef<HTMLElement>>;

private _getSlideElement(slideId: string): HTMLElement {
    return this._slideElements.find(({nativeElement}) => nativeElement.id === slideId)!.nativeElement;
}

WDYT?

Cheers,
Max

[fbasso]

After some discussion with @maxokorokov, it seems that the id are not used the way we thought. At the end, on this last change, I:

• Removed the ids for the indicators (not used)
• Change the slides ids to prefix them by 'slide-' instead of a suffix (initially done by feat(carousel): accessibility #3773, to avoid the id issue (querySelector not possible with id starting with a number)
• Revert the 'should mark the requested slide as active' test (panel id with numbers)

[maxokorokov]

LGTM, thanks!