feat(carousel): Allow to hide navigation items if only one slide #2275
Conversation
|
The tests seems to fail only due to Microsoft Edge and nbg-datepicker. |
src/carousel/carousel.ts
Outdated
| @@ -183,6 +195,10 @@ export class NgbCarousel implements AfterContentChecked, | |||
| } | |||
| } | |||
|
|
|||
| showNavigation(): boolean { return !this.hideNavigation || (this.slides != null && this.slides.length > 1); } | |||
There was a problem hiding this comment.
@wilhelmhb could you please remove the this.slides.length > 1 check (and the corresponding tests(s) based on #2274 (comment) ?
There was a problem hiding this comment.
If I remove the this.slides.length > 1 check, then the navigation items will be hidden, no matter how many slides you have.
If you have several slides, IMHO, it would never make sense to hide these navigations items. Hence the check.
src/carousel/carousel.ts
Outdated
| @@ -183,6 +195,10 @@ export class NgbCarousel implements AfterContentChecked, | |||
| } | |||
| } | |||
|
|
|||
| showNavigation(): boolean { return !this.hideNavigation || (this.slides != null && this.slides.length > 1); } | |||
|
|
|||
| showIndicator(): boolean { return !this.hideIndicator || (this.slides != null && this.slides.length > 1); } | |||
There was a problem hiding this comment.
@wilhelmhb could you please remove the this.slides.length > 1 check (and the corresponding tests(s) based on #2274 (comment) ?
There was a problem hiding this comment.
if we have more than one slide, we probably still want to display the navigation item, right ?
Otherwise the user won't see that he/she can slide through the several slides.
|
@wilhelmhb it looks good to me, generally speaking, could you just please:
Thnx! |
wilhelmhb
force-pushed
the
hide-navigation-carousel
branch
from
edd6373
to
8d3ab17
Compare
|
As navigation items are required when there are several slides, the explicit check on one slide is needed. |
Why? Slides still change with time interval so I can imagine people wanting to have a carousel without navigation, even if there are multiple slides.
I don't think I see why. Once again, I believe that an explicit flag to disable navigation (regardless of number of slides) would be enough (and less confusing at the same time). |
|
I get what you mean, I'll change it ASAP |
…e slide Add parameters to hide navigation arrows and indicators when the carousel contains only one slide Closes ng-bootstrap#2274
wilhelmhb
force-pushed
the
hide-navigation-carousel
branch
from
8d3ab17
to
6f66ac3
Compare
Closes #2274