272 slider component #357
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:
| component: `A wrapping layout to show elements in a carousel and slide them, for example [MucCards](/docs/muccard--docs). | ||
|
|
||
| 🔗 Patternlab-Docs (https://patternlab.muenchen.space/?p=components-slider-quote) | ||
| `, |
There was a problem hiding this comment.
💡 Codebase verification
Ze documentation link ist not accessible!
I have checked ze URL und it returns 401 Unauthorized error. Zis means ze documentation ist either:
- Behind authentication
- Moved to a different location
- No longer available
Please update ze documentation link to ze correct und publicly accessible URL or remove it if ze documentation ist no longer available.
🔗 Analysis chain
Ze documentation link needs verification!
Please ensure zat ze Patternlab-Docs link ist still valid und accessible.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the documentation URL is accessible
curl -s -I "https://patternlab.muenchen.space/?p=components-slider-quote" | head -n 1
Length of output: 113
| const sliderOptions: Options = { | ||
| autoplay: false, | ||
| keyboard: true, | ||
| slideFocus: true, | ||
| lazyLoad: false, | ||
| arrows: false, | ||
| perMove: 1, | ||
| gap: "0", | ||
| type: "slide", | ||
| perPage: 1, | ||
| pagination: false, | ||
| speed: 350, | ||
| mediaQuery: "min", | ||
| breakpoints: { | ||
| 1: { | ||
| perPage: 1, | ||
| gap: "1.5rem", | ||
| }, | ||
| 768: { | ||
| perPage: 1, | ||
| gap: "1.5rem", | ||
| }, | ||
| 1200: { | ||
| perPage: 1, | ||
| gap: "2rem", | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ze slider needs better mobile optimization!
Ze breakpoints configuration ist redundant since all have ze same perPage value. Also, ve are missing important touch options for mobile users!
Here ist how ve can improve it:
const sliderOptions: Options = {
autoplay: false,
keyboard: true,
slideFocus: true,
lazyLoad: false,
arrows: false,
perMove: 1,
gap: "0",
type: "slide",
perPage: 1,
pagination: false,
speed: 350,
+ drag: true,
+ swipe: true,
+ swipeThreshold: 50,
mediaQuery: "min",
breakpoints: {
- 1: {
- perPage: 1,
- gap: "1.5rem",
- },
- 768: {
- perPage: 1,
- gap: "1.5rem",
- },
- perPage: 1,
gap: "2rem",
},
},
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sliderOptions: Options = { | |
| autoplay: false, | |
| keyboard: true, | |
| slideFocus: true, | |
| lazyLoad: false, | |
| arrows: false, | |
| perMove: 1, | |
| gap: "0", | |
| type: "slide", | |
| perPage: 1, | |
| pagination: false, | |
| speed: 350, | |
| mediaQuery: "min", | |
| breakpoints: { | |
| 1: { | |
| perPage: 1, | |
| gap: "1.5rem", | |
| }, | |
| 768: { | |
| perPage: 1, | |
| gap: "1.5rem", | |
| }, | |
| 1200: { | |
| perPage: 1, | |
| gap: "2rem", | |
| }, | |
| }, | |
| }; | |
| const sliderOptions: Options = { | |
| autoplay: false, | |
| keyboard: true, | |
| slideFocus: true, | |
| lazyLoad: false, | |
| arrows: false, | |
| perMove: 1, | |
| gap: "0", | |
| type: "slide", | |
| perPage: 1, | |
| pagination: false, | |
| speed: 350, | |
| drag: true, | |
| swipe: true, | |
| swipeThreshold: 50, | |
| mediaQuery: "min", | |
| breakpoints: { | |
| 1200: { | |
| gap: "2rem", | |
| }, | |
| }, | |
| }; |
src/components/Slider/MucSlider.vue
Outdated
| onMounted(() => { | ||
| if (splide.value && splide.value.splide) { | ||
| splideLength.value = splide.value.length; | ||
| splide.value.splide.on("move", () => { | ||
| if (splide.value) { | ||
| currentSlide.value = splide.value.splide.index; | ||
| emit("changeSlide", splide.value.splide.index); | ||
| } | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Ve need better error handling in ze mounted hook!
Ze initialization could fail und cause runtime errors. Ve should add proper error handling, ja?
Here ist ze fix:
onMounted(() => {
+ try {
if (splide.value && splide.value.splide) {
splideLength.value = splide.value.length;
splide.value.splide.on("move", () => {
if (splide.value) {
currentSlide.value = splide.value.splide.index;
emit("sliderIndexChange", splide.value.splide.index);
}
});
}
+ } catch (error) {
+ console.error("Failed to initialize slider:", error);
+ // Consider emitting an error event here
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onMounted(() => { | |
| if (splide.value && splide.value.splide) { | |
| splideLength.value = splide.value.length; | |
| splide.value.splide.on("move", () => { | |
| if (splide.value) { | |
| currentSlide.value = splide.value.splide.index; | |
| emit("changeSlide", splide.value.splide.index); | |
| } | |
| }); | |
| } | |
| }); | |
| onMounted(() => { | |
| try { | |
| if (splide.value && splide.value.splide) { | |
| splideLength.value = splide.value.length; | |
| splide.value.splide.on("move", () => { | |
| if (splide.value) { | |
| currentSlide.value = splide.value.splide.index; | |
| emit("sliderIndexChange", splide.value.splide.index); | |
| } | |
| }); | |
| } | |
| } catch (error) { | |
| console.error("Failed to initialize slider:", error); | |
| // Consider emitting an error event here | |
| } | |
| }); |
|
@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 |
| const sliderOptions: Options = { | ||
| autoplay: false, | ||
| keyboard: true, | ||
| slideFocus: true, | ||
| lazyLoad: false, | ||
| arrows: false, | ||
| perMove: 1, | ||
| gap: "0", | ||
| type: "slide", | ||
| perPage: 1, | ||
| pagination: false, | ||
| speed: 350, | ||
| mediaQuery: "min", | ||
| breakpoints: { | ||
| 1: { | ||
| perPage: 1, | ||
| gap: "1.5rem", | ||
| }, | ||
| 768: { | ||
| perPage: 1, | ||
| gap: "1.5rem", | ||
| }, | ||
| 1200: { | ||
| perPage: 1, | ||
| gap: "2rem", | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
I would outsource this information into another file to keep the component clean
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.