Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

feat(carousel): hide prev/next if not loop and indexes at bounds #5708

Closed
wants to merge 1 commit into from

Conversation

plondon
Copy link
Contributor

@plondon plondon commented Mar 29, 2016

No description provided.

@@ -1,10 +1,10 @@
<div ng-mouseenter="pause()" ng-mouseleave="play()" class="carousel" ng-swipe-right="prev()" ng-swipe-left="next()">
<div class="carousel-inner" ng-transclude></div>
<a role="button" href class="left carousel-control" ng-click="prev()" ng-show="slides.length > 1">
<a role="button" href class="left carousel-control" ng-click="prev()" ng-show="slides.length > 1" ng-hide="active === 0 && noWrap()">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these changes add two extra watches per slide - can you please investigate combining the ng-show/ng-hide?

@RobJacobs
Copy link
Contributor

I'd prefer ng-if over ng-hide/show. We also need to consider, does it make for a jarring experience with elements being removed rather than just disabling them?

@plondon
Copy link
Contributor Author

plondon commented Mar 29, 2016

Also a fan of disable, that way users can choose the more jarring experience if they want with CSS. Since disabled is not a valid attribute for an anchor tag is it alright to swap <a role="button"> for the regular <button>?

@icfantv
Copy link
Contributor

icfantv commented Mar 29, 2016

@plondon are there accessibility issues with doing that or is that the preferred mechanism? IIRC, using the btn-link class had accessibility ramifications.

@wesleycho
Copy link
Contributor

These need to be condensed into a single function for the show/hide - too much logic in the view here.

@plondon
Copy link
Contributor Author

plondon commented Mar 29, 2016

@icfantv <button> has the aria role="button" by default, so it should act as <a role="button"> from an accessibility perspective. @realalexhomer anything to add here regarding <button> and accessibility? Thanks

@realalexhomer
Copy link

@plondon @icfantv Yes that is correct, screen readers prefer over the a tag so it is better to use it when possible

@plondon plondon force-pushed the carousel-control-logic branch from ac24557 to e70f76b Compare March 29, 2016 23:27
@plondon
Copy link
Contributor Author

plondon commented Mar 29, 2016

Since previous and next buttons are "disabled" by the noWrap logic I think we can leave them as anchor tag's.

Instead of button disabled="true" as we were discussing above I used isPrevDisabled() and isNextDisabled() to toggle a disabled class, leaving it up to the user to hide, show, fade, or whatever the buttons him/herself.

I'll squash these commits if you all agree with this approach.

@plondon plondon force-pushed the carousel-control-logic branch from e70f76b to f2e8730 Compare March 29, 2016 23:32
@wesleycho
Copy link
Contributor

This looks solid to me - anyone else have thoughts?

@plondon plondon force-pushed the carousel-control-logic branch from f2e8730 to fb94d95 Compare March 30, 2016 13:34
@RobJacobs
Copy link
Contributor

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants