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

feat: carousel component using embla carousel #527

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Waishnav
Copy link

No description provided.

Copy link

netlify bot commented Nov 10, 2024

Deploy Preview for kobalte ready!

Name Link
🔨 Latest commit 77eab5d
🔍 Latest deploy log https://app.netlify.com/sites/kobalte/deploys/6741681e07faae0008f4c0eb
😎 Deploy Preview https://deploy-preview-527--kobalte.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 84 (🟢 up 41 from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (no change from production)
SEO: 82 (no change from production)
PWA: 60 (🔴 down 10 from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@Waishnav Waishnav changed the title feat: tried implementing it using embla carousel feat: carousel component using embla carousel Nov 10, 2024
@MengLinMaker
Copy link
Contributor

@Waishnav Might want to check if using embla-carousel dependency is allowed.

From what I understand, UI primitive libraries like this one will generally avoid adding dependencies.

@Waishnav
Copy link
Author

@Waishnav Might want to check if using embla-carousel dependency is allowed.

From what I understand, UI primitive libraries like this one will generally avoid adding dependencies.

Hmm make sense
@jer3m01, I'd love to hear your thoughts and any suggestions on how we might proceed with implementing this. Thank you both for your insights!

@jer3m01
Copy link
Member

jer3m01 commented Nov 13, 2024

Thanks for the draft!

I think this could be a good component, rewriting an entire carousel is out of scope for now.
Using embla (specifically https://www.embla-carousel.com/get-started/solid/) should be the way forward.

The API should ideally be fully wrapped in Kobalte with props on the Carousel.Root for all embla options. (The end user should not access embla themselves).
Only the interactivity should be left to embla, so that the entire layout of the component is generated during SSR.

Looking forward to this, good luck!

@Waishnav Waishnav marked this pull request as ready for review November 16, 2024 19:12
@Waishnav
Copy link
Author

Hey @jer3m01,

I've implemented all the suggestions from your previous message.

The Carousel component now uses Embla under the hood and exposes it through the options and plugins props, allowing users to enjoy the flexibility and power of Embla while retaining full control.

With Kobalte's WAI-ARIA compliance and unstyled component philosophy, users can now compose their own custom carousel components as needed.

That said, I’m still relatively new to writing composable UI components in this pattern. I’d greatly appreciate it if you could review it thoroughly and share any feedback or suggestions for improvement. Looking forward to iterating further based on your input!

@Waishnav
Copy link
Author

one more thing, should we add the keyboard interaction support just like we have for Tabs component?

Copy link
Member

@jer3m01 jer3m01 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

should we add the keyboard interaction support

Yes it would be good to support prev/next with arrow and first/last with home/end.

There's also some small changes to be done.

@DeadlyMissile
Copy link

DeadlyMissile commented Nov 19, 2024

Rapidly clicking on the buttons don’t move the slides faster. They seem to be stuck.

Also examples are broken on my phone. I think it is because you are using css nesting which is relatively new.

@Waishnav
Copy link
Author

Yes it would be good to support prev/next with arrow and first/last with home/end.

Hey, I've added the keyboard interaction for better accessibility. Please let me know if there is any further changes needed

@Waishnav Waishnav requested a review from jer3m01 November 23, 2024 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants