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): new component #927

Merged
merged 12 commits into from
Jan 22, 2024
Merged

feat(Carousel): new component #927

merged 12 commits into from
Jan 22, 2024

Conversation

Gobler
Copy link
Contributor

@Gobler Gobler commented Nov 7, 2023

πŸ”— Linked issue

#550

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Add new full customizable carousel component for resolves #550

Features:

  • Navigation with arrows is optional
  • Navigation with indicators is optional
  • Slot for list items has been prepared
  • Slot for indicators has been prepared
  • Slot for arrow navigation has been prepared
  • RWD support has been prepared (using tailwind classes)
  • Configurable number of slides displayed
  • Dynamically adjusted number of indicators (based on the number of slides to be displayed)
  • Native solutions were used as much as possible
  • Full support for mobile devices

🎨 Preview

Capture-2023-11-07-215722
Capture-2023-11-07-215824

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

Copy link

vercel bot commented Nov 7, 2023

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Updated (UTC)
ui βœ… Ready (Inspect) Visit Preview Jan 22, 2024 4:40pm

@Gobler
Copy link
Contributor Author

Gobler commented Nov 10, 2023

@benjamincanac what do you thing about this?

@ddahan
Copy link

ddahan commented Nov 25, 2023

I hope this will be merged soon.

@benjamincanac
Copy link
Member

Still thinking about this but I'm not sure there is a point to include this in @nuxt/ui as there are lots of great vue librairies that does the job. I'd put this in the same category as drag'n'drop, calendar / date picker, graphs / charts, etc.

@ddahan
Copy link

ddahan commented Nov 29, 2023

Hi @benjamincanac!

Very interesting topic. I will give my very personal opinion on this.

For me, one of the main advantage of installing a UI Library is to have the maximum of built in UI components in it.
The more I need to rely on additional external libraries, the more it feels clunky to me, because I have to manage different external packages, with different documentations to read, with different risks of being unmaintained over time.

Besides, it adds on "paradox of choices". I don't want to spend time searching and comparing external libraries when I just want to add a basic carousel to my project on a single page. It just not worths it.

While I can absolutely understand the point for charts because it's very specific and requires a huge work on itself, it's less obvious for carousel which is handled by most of UI libraries.

In the end, I hate my project being a patchwork of small singled-purpose libraries. The more I have this "all-batteries-included" feeling, the more I feel confident. And I have the very subjective feeling that people using Nuxt (and Nuxt UI) prefer ease over the rest.

A good philosophy could be to propose simple Nuxt UI carousel for basic users and tell to power users: "if you need a more advanced carousel, take a look to library X".
Forcing basic users (maybe 80-90% of overall users) to use a dedicated library would be detrimental to them.
But it would be ok for 10% power users to install an additional library.

Copy link
Member

I agree with you, a Carousel might not be as complex as charts or datepickers! What about this #1027 then? I'm not entirely sure where to draw the line on what should be or not included in the library..

I'm currently focused on other topics but I'll try to review this asap!

@ddahan
Copy link

ddahan commented Nov 29, 2023

#1027 looks like the same than Carousel but dedicated to images. Not sure it deserves a whole component for it.
Don't want to sound rude with PrimeVue guys, but I have the feeling they love making new components that are a slight variation of another, just to look bigger.

I'm not entirely sure where to draw the line

I understand...
Plus I can understand the frustration to integrate to nuxt UI a less accomplished carousel than swiper.js.

IMO having a built-in carousel more or less the same than this one in Nuxt UI would still be great as it would give the choice to users:

  • Need a standard carousel for basic needs? We got you covered with no extra setup
  • Need a very customizable carousel with advanced features? Take a look to swiper.js or whatever is cutting edge component.

Copy link
Member

Sounds good to me! 😊

@Gobler
Copy link
Contributor Author

Gobler commented Nov 30, 2023

I've been away for a while and there's so much going on here ;)

In my opinion, we should provide the basic elements of the user interface (as the name of the package suggests ;p) and it seems to me that the carousel is such an element - just like a table or forms (for them there are also dedicated more advanced solutions - FormKit, vue-good-table etc)

If someone will need a more advanced solution then they will consciously choose it or customize/expand this basic one (depending on actual needs)

So the question @benjamincanac - should I update this request and we will want to use it or do we let it go for now?

If we want to use it, are there any comments/suggestions on the code itself or how it works?

Copy link
Member

I haven't reviewed it in detail yet but you can fix the conflict on the ui.config.ts (which has been split in multiple files). We'll include this in the next release yes 😊

@Gobler
Copy link
Contributor Author

Gobler commented Nov 30, 2023

I haven't reviewed it in detail yet but you can fix the conflict on the ui.config.ts (which has been split in multiple files). We'll include this in the next release yes 😊

Ok, I'll try to figure it out tonight or tomorrow night

@ineshbose
Copy link
Member

I haven't reviewed it in detail yet but you can fix the conflict on the ui.config.ts (which has been split in multiple files). We'll include this in the next release yes 😊

Ok, I'll try to figure it out tonight or tomorrow night

That's done for you. (currently going over PRs affected by the ui.config.ts change)

@benjamincanac
Copy link
Member

@Gobler Would you mind adding the carousel.md page for the docs?

@benjamincanac
Copy link
Member

@Gobler I've made a few changes to your PR if you want to take a look!

@benjamincanac benjamincanac merged commit f37b043 into nuxt:dev Jan 22, 2024
2 checks passed
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.

[Component] Carousel
4 participants