search

bestguy / sveltestrap

Bootstrap 4 & 5 components for Svelte
https://sveltestrap.js.org
MIT License
1.3k stars 183 forks source link

Missing Carousel #30

Open bestguy opened 5 years ago

bestguy commented 5 years ago

Carousel is not implemented: https://getbootstrap.com/docs/4.3/components/carousel/

lucianoratamero commented 4 years ago

hey, @bestguy , good evening!

I spent some time trying to figure the best approach on how to implement this (I'm still new to svelte), and I stumbled into this project here: https://github.com/thednp/bootstrap.native/

they seem to reimplement bootstrap plugins without jquery. it could save us some time if we could make use of that to implement the Carousel, since the components would only need to render slots and the Carousel component could call their script. I'm starting a little PoC here to see if things behave as intended.

in your opinion, should I keep trying to use it, or do you want to keep things native to svelte?

bestguy commented 4 years ago

Hi @lucianoratamero, thank you for the help. Yes I have looked at that in the past, however there were some philosophic differences with the library's author in that he felt is was okay to deviate from Bootstrap's behavior, and generally closing all issues rapidly. Made me a little nervous to depend on it directly, but if you have a start that would be great!

It's under MIT license so we could also use/fork parts that are helpful for some components, but in any case please let me know what you find!

bestguy commented 4 years ago

Also if it helps, Reactstrap is a React version of Bootstrap components, and their implementation might be good inspiration/guidance: https://github.com/reactstrap/reactstrap/blob/master/src/Carousel.js

daytonlowell commented 4 years ago

I'm playing around with implementing this. As far as the API is concerned, how much do we want to match what the BS jQuery JS does? Do we need to support data- attributes like data-slide="prev"?

bestguy commented 4 years ago

Thanks @daytonlowell , was thinking the component-based approach like Reactstrap Carousel uses: https://reactstrap.github.io/components/carousel/

would be a good approach for this.

daytonlowell commented 4 years ago

I made a commit to my fork to add support for Carousel. Not ready to make a PR quite yet, but feel free to take a look and comment.

https://github.com/daytonlowell/sveltestrap/commit/88eedd867b31c90dacb54dde2dced636443864b8

Here's the state of things:

I'm not exactly sure how to handle transitions. I played around with wrapping the template code for CarouselItem in a {#if} block and played with some of the Svelte transitions. It was ok, but not great.

daytonlowell commented 4 years ago

Keyboard support added in https://github.com/daytonlowell/sveltestrap/commit/0967235f54bb22ba6e4392877fe08837d386e834

We'll probably need to do something smarter so that the keyboard presses don't effect all carousels on the page?

bestguy commented 4 years ago

This is great @daytonlowell thank you! Let me pull and check it out later today, I appreciate the help.

daytonlowell commented 4 years ago

Added initial support for ride, interval, and pause.

https://github.com/daytonlowell/sveltestrap/commit/bdd866257c2ef8f1e3098e94b76a55dbbc2ef2e1

bestguy commented 4 years ago

Looking great @daytonlowell !, what further changes are needed before PRing/merging?

daytonlowell commented 4 years ago

Do we care about touch support right now? That's going to be harder. I don't really have any experience with the touch events. I assume we don't want to depend on hammer.js.

We still need to figure out how to support transition animations between items in a way that looks like the default BS animation(they support fade and slide).

Finally, I'd like it if we could better handle the behavior when moving between slides via keyboard when ride is enabled and via the controls when ride is enabled and pause isn't. It gets kinda wonky.

Maybe we need to temporarily disable ride for some amount of time when they perform a manual interaction. ride is already temporarily disabled when they're moused over the carousel if pause is enabled.

bestguy commented 4 years ago

Great update thanks. We can leave this pending and open for the near term, and see if we can add those. If not we can always release whatever is stable and leave the other points as issues for future releases

stefanuddenberg commented 3 years ago

Is there any progress on adding support for transitions?

daytonlowell commented 3 years ago

Not yet. Always looking for contributors though!

bwesth commented 3 years ago

Simply copy-pasting the carousel component from sveltestrap yields this TS error

Type '{ items: string[]; activeIndex: number; }' is not assignable to type 'IntrinsicAttributes & ICarouselProps & LocalSvelteProps'. Type '{ items: string[]; activeIndex: number; }' is missing the following properties from type 'ICarouselProps': next, previous ts(2322)

Removing the ts dependency removes the errors, but I'm thinking this is not a desirable solution. Are the examples updated?

bestguy commented 3 years ago

Hi @bwesth ,

This seems to work fine for me in TypeScript:

https://svelte.dev/repl/c7f5e86874c549809f4cfcc2829f0714?version=3.35.0

Can you confirm your build/imports?

bestguy commented 3 years ago

Oh it looks like we mark next and previous as required in TS: https://github.com/bestguy/sveltestrap/blob/master/src/Carousel.d.ts#L8

That may not be correct, but let me confirm soon. If not, we can release a new build with the correct types.

bestguy commented 3 years ago

Hi @bwesth ,

I think this should be corrected by: https://github.com/bestguy/sveltestrap/pull/241/files

Will release a new version this weekend, thanks for the heads up.

cstayyab commented 3 years ago

Anyone working on fluid option for carousel? It sets class "container" of parent wrapper of Carousel but if we can provide option for container-fluid that would be great. If anyone isn't working on this I would be happy to implement this.

cc: @daytonlowell @bestguy