-
Notifications
You must be signed in to change notification settings - Fork 196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Yield functions to switch active slides in <BsCarousel> #1300
base: master
Are you sure you want to change the base?
Conversation
Thanks a lot for working on this! Can you please add documentation and tests as well? |
Happy to add documentation however sure where/what to add for documentation. Here is how I am using it (requires a third party library). <BsCarousel as |carousel|>
{{#each @model.imageUrls as |imageUrl|}}
<carousel.slide>
<img
src={{imageUrl}}
{{recognize-gesture "swipe"}}
{{on "swipe" (fn this.swipe carousel)}} />
</carousel.slide>
{{/each}}
</BsCarousel> with a recognizer export default {
include: [],
exclude: [],
options: { threshold: 25, direction: typeof Hammer === 'undefined' ? '' : Hammer.DIRECTION_HORIZONTAL },
recognizer: 'swipe'
}; |
The yielded functions should be mentioned in the API docs: https://github.com/kaliber5/ember-bootstrap/blob/c35a790696961d1666099262518664027104f923/addon/components/bs-carousel.js#L15-L64 The documentation should cover what they are meant for and how they work. Additionally a simple usage example should be given. Ember Bootstrap's API documentation currently does not provide first-class support for documenting arguments and yielded values / functions. Yielded values and function are currently documented in the overview text at the beginning. Please have a look at documentation for Please don't get confused that the component is missing in deployed API docs. This is a bug, which I just noticed. Opened #1301 to track that one. |
Thanks for adding the documentation at @nolman. Looks good to me. Only tests are missing to get this merged from my side. @simonihmig Do you have any concerns with introducing this new public API? |
No, that addition looks good. Thanks @nolman! Yes, only some test similar to existing ones would be required to merge this. |
@nolman Any chance you can add tests for this one? Feel free to reach out to me on Discord if you have any questions. |
@nolman Would love to get this in. Do you think you will have some time to add the missing tests? |
* `toPrevSlide`: function to change change the carousel to the previous slide. | ||
* `toNextSlide`: function to change change the carousel to the next slide. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* `toPrevSlide`: function to change change the carousel to the previous slide. | |
* `toNextSlide`: function to change change the carousel to the next slide. | |
* `toPrevSlide`: function to change the carousel to the previous slide. | |
* `toNextSlide`: function to change the carousel to the next slide. |
This exposes the
toNextSlide
andtoPrevSlide
functions so that they can be called. I am doing this so that I can mixing ember-gestures (hammer.js) to add support for left/right swipe events. This can enable the requested functionality in#613.