Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nolman
Copy link

@nolman nolman commented Oct 20, 2020

This exposes the toNextSlide and toPrevSlide 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.

@jelhan
Copy link
Contributor

jelhan commented Oct 20, 2020

Thanks a lot for working on this! Can you please add documentation and tests as well?

@nolman
Copy link
Author

nolman commented Oct 20, 2020

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 app/ember-gestures/recognizers/swipe.js

export default {
  include: [],
  exclude: [],
  options: { threshold: 25, direction: typeof Hammer === 'undefined' ? '' : Hammer.DIRECTION_HORIZONTAL },
  recognizer: 'swipe'
};

@jelhan
Copy link
Contributor

jelhan commented Oct 20, 2020

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 <Form::Element> and <Modal> for examples how that is done.

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.

@jelhan
Copy link
Contributor

jelhan commented Oct 21, 2020

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?

@jelhan jelhan changed the title add handle to prev/next slide Yield functions to switch active slides in <BsCarousel> Oct 21, 2020
@simonihmig
Copy link
Contributor

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.

@jelhan
Copy link
Contributor

jelhan commented Nov 2, 2020

@nolman Any chance you can add tests for this one? Feel free to reach out to me on Discord if you have any questions.

@jelhan
Copy link
Contributor

jelhan commented Nov 30, 2020

@nolman Would love to get this in. Do you think you will have some time to add the missing tests?

Comment on lines +35 to +36
* `toPrevSlide`: function to change change the carousel to the previous slide.
* `toNextSlide`: function to change change the carousel to the next slide.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `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.

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

Successfully merging this pull request may close these issues.

None yet

3 participants