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

feat(carousel): exposing next and prev on the controller. #4853

Closed
wants to merge 1 commit into from
Closed

feat(carousel): exposing next and prev on the controller. #4853

wants to merge 1 commit into from

Conversation

jsdevel
Copy link
Contributor

@jsdevel jsdevel commented Nov 9, 2015

No description provided.

@icfantv
Copy link
Contributor

icfantv commented Nov 9, 2015

Interesting note that we don't currently have functional tests in place for testing that next() acutally navigates to the next slide. Same for prev(). @Foxandxss, is this something you can add to your test sweep?

@Foxandxss
Copy link
Contributor

@icfantv actually we do. https://github.com/angular-ui/bootstrap/blob/master/src/carousel/test/carousel.spec.js#L175-L195

Those click are calling the appropriate methods.

About this PR. What is the use case? and I like better tests (more than checking that there is a function, those are poor).

@jsdevel
Copy link
Contributor Author

jsdevel commented Nov 9, 2015

About this PR. What is the use case?

I have an element directive that uses uib-carousel in it's template. I wrote another attribute directive that requires my element directive and uib-carousel. The attribute directive decorates .select on uib-carousel and notifies my element directive so it can hide/show the left|right controls accordingly. My logic would be simpler if I could decorate next|prev instead.

... and I like better tests (more than checking that there is a function, those are poor).

I couldn't agree more. The only reason I did this was because $scope.next and $scope.prev were already being tested. Perhaps I should update the tests to verify that ctrl.next === $scope.next?

Thanks for the great work on this lib!

@jsdevel
Copy link
Contributor Author

jsdevel commented Nov 9, 2015

Perhaps I should update the tests to verify that ctrl.next === $scope.next?

Just pushed this to the PR, so it's now verifying that the methods are one and the same between ctrl and scope. This allows the existing tests for scope.(next|prev) to verify that the addition will work as expected.

About this PR. What is the use case?

Also, I've had edge cases where the markup in uib-carousel doesn't meet my needs, so I ended up grabbing .next|.prev off of scope to make my own custom controls, but this was dirty. Easier to require the controller and avoid $timeout to wait for the control elements to be present.

@icfantv
Copy link
Contributor

icfantv commented Nov 9, 2015

Ahhhh, my bad. I was looking for direct invocations and not, you know, actually clicking the thing that matters...the navigational elements.

@wesleycho
Copy link
Contributor

I'm ok with this PR, but I'm not sure tests really make any sense, or at least these anyhow.

@jsdevel
Copy link
Contributor Author

jsdevel commented Nov 10, 2015

I'm ok with this PR, but I'm not sure tests really make any sense, or at least these anyhow.

I'm open to suggestions. $scope.prev and $scope.next are tested extensively in other parts of the spec file. To avoid duplication, the tests as they currently stand are just asserting that the controller methods reference the $scope methods.

@wesleycho
Copy link
Contributor

I was thinking that this is ok with no extra tests - I'll wait for @Foxandxss's opinion here though.

@jsdevel
Copy link
Contributor Author

jsdevel commented Nov 10, 2015

Gotcha.

@wesleycho wesleycho added this to the 1.0.0 milestone Nov 10, 2015
@wesleycho wesleycho closed this in 9b80ee1 Nov 10, 2015
@jsdevel jsdevel deleted the exposing-next-prev-on-carousel-controller branch January 10, 2016 06:18
@jsdevel
Copy link
Contributor Author

jsdevel commented Jan 10, 2016

Thanks for merging this!

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.

4 participants