feat(carousel): [carousel]modify smb theme and add props #2125
Conversation
WalkthroughThe changes introduce a new Changes
Suggested labels
Tip OpenAI O1 model for chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
examples/sites/demos/pc/app/carousel/close-loop.vue (1)
2-2: Consider using a dynamic value for thedisabledproperty.The changes to the
<tiny-carousel>component properties are mostly fine:
- Changing the
loopproperty from:loop="false"toloopenables the looping functionality by default, which aligns with common expectations.- Adding the
:disabled="true"property disables the interactivity of the carousel, which is likely done for demonstration purposes.However, consider using a dynamic value for the
disabledproperty to allow flexibility in the example. Hardcoding it totruemeans the carousel will always be disabled in this example.-<tiny-carousel height="150px" loop :disabled="true" arrow="always"> +<tiny-carousel height="150px" loop :disabled="isDisabled" arrow="always">In the script section, you can define a data property
isDisabledand set its value as needed:export default { data() { return { isDisabled: true } }, // ... }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- examples/sites/demos/apis/carousel.js (1 hunks)
- examples/sites/demos/pc/app/carousel/card-show-composition-api.vue (3 hunks)
- examples/sites/demos/pc/app/carousel/card-show.spec.ts (1 hunks)
- examples/sites/demos/pc/app/carousel/card-show.vue (3 hunks)
- examples/sites/demos/pc/app/carousel/close-loop-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/carousel/close-loop.spec.ts (2 hunks)
- examples/sites/demos/pc/app/carousel/close-loop.vue (1 hunks)
- examples/sites/demos/pc/app/carousel/webdoc/carousel.js (1 hunks)
- packages/theme/src/carousel/index.less (2 hunks)
- packages/theme/src/carousel/vars.less (2 hunks)
- packages/vue/src/carousel/src/index.ts (1 hunks)
- packages/vue/src/carousel/src/pc.vue (3 hunks)
Additional comments not posted (33)
examples/sites/demos/pc/app/carousel/card-show.spec.ts (2)
7-8: LGTM!The change simplifies the interaction logic by directly referencing and clicking the second button (
btnRight) without the intermediate step of clicking a div. This improves the clarity and efficiency of the test.
13-13: LGTM!The assertion enhances the validation of the button's state following the interaction by checking that the button has a specific CSS property (
fillset to 'rgb(194, 194, 194)') after the click action. This ensures the button's appearance is as expected.examples/sites/demos/pc/app/carousel/close-loop-composition-api.vue (1)
2-2: Verify the intended behavior and user experience.The changes to the
tiny-carouselcomponent enable continuous looping of items while disabling user interaction. This combination will result in an automatic transition of items without the ability for users to manually control the navigation.Please confirm if this is the desired behavior and user experience for this specific use case.
Consider adding a comment to clarify the purpose of these changes, such as:
<!-- Enable continuous looping and disable user interaction --> <tiny-carousel height="150px" loop :disabled="true" arrow="always">examples/sites/demos/pc/app/carousel/close-loop.spec.ts (2)
12-12: LGTM!The assertion change aligns with the expected behavior of displaying the left arrow when the carousel is not in a looping mode, allowing navigation to the previous slide.
21-21: LGTM!The assertion change aligns with the expected behavior of displaying the right arrow when the carousel is not in a looping mode, allowing navigation to the next slide.
packages/vue/src/carousel/src/index.ts (1)
49-52: LGTM!The addition of the
disabledprop is a valid enhancement to the carousel component. It allows users to control the interactivity of the carousel based on their specific requirements.The prop is correctly typed as a Boolean and has an appropriate default value of
false. The change is consistent with the existing code structure and follows the component's prop declaration pattern.packages/theme/src/carousel/vars.less (4)
39-39: LGTM!The change to the active indicator text color is consistent with the summary and does not introduce any issues.
40-41: LGTM!The new variable for the hover state color of the indicator icons enhances user interaction feedback and does not introduce any issues.
79-79: LGTM!The change to the background colors for the left and right arrows allows for more dynamic theming based on the overall application style and does not introduce any issues.
Also applies to: 81-81
83-85: LGTM!The new variables for the disabled state of the arrow buttons provide visual cues, improving accessibility and user experience. They do not introduce any issues.
packages/vue/src/carousel/src/pc.vue (5)
27-31: LGTM!The class binding change correctly implements the disabled state for the left arrow button. It adds the
tiny-carousel__arrow-disabledclass when thedisabledprop is true and the active index is at the first item (index 0). This visually indicates when the left arrow button is disabled.
32-32: LGTM!The
disabledattribute change correctly implements the disabled state for the left arrow button. It disables the button when theloopprop is true, thedisabledprop is true, and the active index is at the first item (index 0). This prevents user interaction with the left arrow button when it is not applicable.
47-51: LGTM!The class binding change correctly implements the disabled state for the right arrow button. It adds the
tiny-carousel__arrow-disabledclass when thedisabledprop is true and the active index is at the last item (index equal tostate.items.length - 1). This visually indicates when the right arrow button is disabled.
52-52: LGTM!The
disabledattribute change correctly implements the disabled state for the right arrow button. It disables the button when theloopprop is true, thedisabledprop is true, and the active index is at the last item (index equal tostate.items.length - 1). This prevents user interaction with the right arrow button when it is not applicable.
108-108: LGTM!The addition of the
disabledprop to the component's exported properties is correct. It allows thedisabledstate to be controlled externally by the parent component or the application using the carousel component. This enhances the flexibility and reusability of the carousel component.examples/sites/demos/pc/app/carousel/card-show-composition-api.vue (7)
2-2: LGTM!The changes to the
tiny-carouselcomponent's properties are appropriate:
- Setting
arrowto "never" andindicator-positionto "none" removes the default navigation elements.- The
refproperty allows accessing the carousel instance in the script.- Binding
:initial-indextoindex - 1sets the initial active item based on theindexvalue.
3-10: LGTM!The new button layout for carousel navigation is well-implemented:
- The "previous" and "next" buttons provide an intuitive way to navigate the carousel.
- Conditionally disabling the buttons based on
disabledLeftanddisabledRightprevents navigating beyond the available items.- The
prevandnextmethods are appropriately called on button click events to update the carousel's active item.
13-13: LGTM!The change to the
v-fordirective is appropriate:
- Iterating over
state.cardData.lengthallows rendering a<div>element for each item instate.cardData.- The
numvariable represents the current iteration index and is used to conditionally render the card layout based on the current position.
Line range hint
34-141: LGTM!The changes in the script section are well-structured and provide the necessary functionality:
- The
index,disabledLeft,disabledRight, andcarouselRefvariables are properly declared usingref().- The
nextandprevmethods update theindexvalue within the valid range and trigger the corresponding carousel navigation.- The
disableStatusmethod correctly determines the disabled state of the navigation buttons based on the currentindexvalue.- The
onMountedlifecycle hook ensures that the button states are initialized when the component is mounted.
144-168: LGTM!The styling changes to the
.carousel-item-demoand.card-dspclasses are appropriate:
- The changes enhance the layout and alignment of the carousel items and cards.
- The
display,justify-content,align-items, andheightproperties ensure proper centering and height of the elements.
169-176: LGTM!The styling changes to the
.btn-layoutclass are appropriate:
- The changes position the button layout relative to the carousel using the
position,top, andz-indexproperties.- The
display,justify-content, andpaddingproperties ensure proper spacing and alignment of the navigation buttons.
177-195: LGTM!The styling changes to the
/deep/ .tiny-button.tiny-button--text.tiny-button.is-only-iconselector are appropriate:
- The changes customize the appearance of the navigation buttons by removing the border, adjusting the icon size, and setting the fill color of the icons.
- The disabled state and hover effects are properly handled to provide visual feedback to the user.
- The changes ensure a consistent and intuitive user experience when interacting with the navigation buttons.
examples/sites/demos/pc/app/carousel/card-show.vue (6)
2-10: LGTM!The changes introduce a custom navigation layout for the carousel using "previous" and "next" buttons. The button states are managed using the
disabledLeftanddisabledRightproperties, and theprev()andnext()methods handle the navigation logic. The implementation looks good.
13-13: LGTM!The change dynamically renders the card layout based on the
cardDataarray using av-fordirective. This improves the flexibility of the component and the implementation is correct.
34-42: LGTM!The changes import and register the necessary components for the carousel, card, button, and icons. The imports and component registrations are correct.
Line range hint
46-137: LGTM!The changes introduce new data properties and methods to handle carousel navigation and button states. The
next()andprev()methods correctly update theindexand call the corresponding methods on the carousel ref. ThedisableStatus()method correctly updates the button states based on the currentindex. Themounted()hook ensures that the button states are initialized when the component is mounted. The implementation looks good.
142-146: LGTM!The styles correctly center the carousel items horizontally using
display: flexandjustify-content: center. The implementation looks good.
150-193: LGTM!The styles correctly align the cards vertically using flexbox, set the width, height, and margin of the cards, position and style the navigation buttons, and customize the appearance of the buttons and icons. The implementation looks good.
packages/theme/src/carousel/index.less (3)
62-64: LGTM!The hover effect on the SVG element within the carousel arrow enhances the user experience by providing visual feedback. The use of a CSS variable for the hover color allows for easy customization.
92-98: Great work on styling the disabled carousel arrows!The new
.@{carousel-prefix-cls}__arrow-disabledclass effectively communicates the non-interactive state of disabled carousel arrows through distinct visual styles. The use of CSS variables for the background color and SVG fill color allows for easy customization. Thenot-allowedcursor style further enhances the user experience by clearly indicating the disabled state.
100-100: Nice improvement to the hover effect on carousel arrows!By adding the
:not(.@{carousel-prefix-cls}__arrow-disabled)pseudo-class to the selector, the hover effect is now only applied to enabled carousel arrows. This ensures that the disabled arrow styles are not overridden when hovered over, maintaining a consistent visual representation of the disabled state. Great attention to detail in improving the user experience!examples/sites/demos/pc/app/carousel/webdoc/carousel.js (1)
55-57: LGTM!The changes accurately clarify the behavior of the carousel when both the
loopanddisabledattributes are set totrue. The updated description helps developers understand how these attributes interact to restrict cyclic navigation.examples/sites/demos/apis/carousel.js (1)
114-128: LGTM!The new
disabledproperty is a valuable addition to the carousel component's configuration. It enhances the component's flexibility by allowing developers to control the interactivity of the carousel arrows based on their specific requirements.The property is correctly defined with the following attributes:
- Name:
disabled- Type:
boolean- Default value:
false- Description: Provided in both Chinese and English, clearly explaining its functionality
Moreover, the property is appropriately categorized under the 'pc' mode and associated with the relevant 'close-loop' demo, ensuring proper integration within the existing configuration structure.
Overall, the changes are well-implemented and align with the component's design principles.
James-9696
added
documentation
James-9696
changed the title
2 participants
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Release Notes
New Features
disabledproperty for the carousel component, allowing users to disable navigation arrows.Improvements
loopanddisabledattributes.Bug Fixes