✨ 272 slider component - #357
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughZe project has undergone a significant enhancement by introducing a new slider component system. Ze changes include adding ze Splide library dependency, creating a Changes
Sequence DiagramsequenceDiagram
participant User
participant MucSlider
participant MucSliderItem
participant Splide Library
User->>MucSlider: Interact with Slider
MucSlider->>Splide Library: Initialize Slider
Splide Library-->>MucSlider: Slider Ready
User->>MucSlider: Navigate Slides
MucSlider->>Splide Library: Change Slide
Splide Library-->>MucSliderItem: Update Active Slide
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
# Conflicts: # package-lock.json
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/components/Slider/MucSliderItem.vue (1)
11-14: Ze slot documentation könnte be more descriptiv!Ze current documentation ist very basic. Consider adding more details about:
- Expected content types
- Any size restrictions
- Example usage
/** - * Elements can be put into this slot. + * Slot for slide content such as MucCard, images, or any other components. + * Content should maintain consistent dimensions for optimal display. + * @example + * <MucSliderItem> + * <MucCard title="Example" /> + * </MucSliderItem> */src/components/Slider/MucSlider.stories.ts (1)
22-39: Ze template could use some improvements!
- Consider using :key with unique identifier instead of index:
-<MucSliderItem v-for="index in 5" :key="index"> +<MucSliderItem v-for="index in 5" :key="`slide-${index}`">
- Ze example ist very basic. Consider adding:
- Different card variations
- Different content types
- Example with navigation options
Also, it vould be gut to add more stories showing different use cases:
export const WithImages = () => ({ components: { MucSlider, MucSliderItem }, template: ` <MucSlider> <MucSliderItem v-for="index in 3" :key="'image-'+index"> <img :src="'https://picsum.photos/400/300?random='+index" alt="Sample image" /> </MucSliderItem> </MucSlider> ` });src/components/Slider/MucSlider.vue (2)
9-13: Gut implementation of ze slider section!Ze accessibility attributes sind very gut! However, ve could improve ze loading state handling.
Consider adding a loading state like zis:
<section class="m-slider m-slider--visible-preview" aria-label="Slider mit Elementen" + :aria-busy="isLoading" data-m-slider-splide="m-slider-comment" > + <div v-if="isLoading" class="m-slider__loading"> + Loading... + </div>
62-68: Ze event name could be more specific!Ze 'changeSlide' event name ist a bit generic. In ze component library, ve should use more specific names.
Consider renaming like zis:
- changeSlide: [index: number]; + sliderIndexChange: [index: number];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
package.json(1 hunks)src/components/Slider/MucSlider.stories.ts(1 hunks)src/components/Slider/MucSlider.vue(1 hunks)src/components/Slider/MucSliderItem.vue(1 hunks)src/components/Slider/index.ts(1 hunks)src/components/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/Slider/index.ts
🔇 Additional comments (3)
src/components/Slider/MucSliderItem.vue (1)
1-5: Ze template structure looks gut!
Ze component correctly wraps ze SplideSlide component und provides a slot for content injection.
src/components/index.ts (1)
24-24: Ja, ze exports look ordentlich!
Ze new slider components are properly imported und exported, maintaining ze alphabetical order.
Also applies to: 52-53
package.json (1)
80-81: 🛠️ Refactor suggestion
Achtung! Ze version of @splidejs/vue-splide ist not ze latest!
Ze current version 0.6.12 ist quite old. Ze latest version ist 1.0.1 mit many improvements und bug fixes. Ve should upgrade, ja?
Let me check ze security advisories:
|
@lehju I needed to use the fork-Version of vue-splide as the project isn't being actively maintained anymore (read more here Splidejs/vue-splide#92 (comment)). The alternative was to use the node tool "patch-package" but that was definitely not a cleaner solution. |
Should we even use a project like this if it is not actively maintained? What happens if we need to update some peer-dependencies ... are there other libraries we can consider? |
There probably are, but we are forced to use it as this is what the MDE uses and we shouldn't be reinventing the wheel here. In the long term, when MDE publishes the components as webcomponents we won't need to include the library in our lib anymore. For now it will be the best solution we have. |
|
understandable |
# Conflicts: # package-lock.json # package.json
|
🎉 This PR is included in version 2.1.0-beta.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
Description
Add slider component
Reference
Issues #272
Summary by CodeRabbit
New Features
MucSlider) utilizing the Splide library for enhanced carousel functionality.MucSliderItem) for customizable slide content.MucSlidercomponent.Chores
@splidejs/vue-splideversion0.6.12.