Refactor slider to use notched outline and add required and invalid props #3626
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto incremental 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 📝 WalkthroughWalkthroughThe slider component was updated by refactoring its rendering to use a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Slider
participant NotchedOutline
participant HelperLine
User->>Slider: Interacts with slider
Slider->>NotchedOutline: Passes label, required, invalid, disabled, readonly, value
NotchedOutline->>Slider: Renders content slot (slider input, track, etc.)
Slider->>HelperLine: Passes invalid property for helper text rendering
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the slider component by adding required and invalid state support, aligning it with other form controls. The changes replace the basic label element with a notched outline component to provide consistent styling and validation states.
- Adds
requiredandinvalidproperties to the slider component - Replaces basic label with
limel-notched-outlinecomponent for consistent form styling - Updates helper line to display invalid state styling
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/components/slider/slider.tsx | Adds required/invalid props and replaces label with notched outline component |
| src/components/slider/slider.scss | Updates styling for new notched outline layout and removes old label styles |
| etc/lime-elements.api.md | Updates TypeScript API definitions to include new required and invalid properties |
Kiarokh
added
visual design
coderabbitai
bot
changed the title
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3626/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (2)
src/components/slider/slider.scss(1 hunks)src/components/slider/slider.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/components/**/*.scss
📄 CodeRabbit Inference Engine (.cursor/rules/css-class-names-in-components-using-shadow-dom.mdc)
Do not use BEM-style class names in CSS for components, as components use shadow-DOM.
Files:
src/components/slider/slider.scss
**/*.{tsx,scss}
⚙️ CodeRabbit Configuration File
**/*.{tsx,scss}: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
Files:
src/components/slider/slider.scsssrc/components/slider/slider.tsx
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/wrap-multiple-jsx-elements-in-host-not-in-array.mdc)
When returning multiple JSX elements from the
rendermethod, never wrap them in an array literal. Instead, always wrap them in the special<Host>element. When there is already a single top-level element, it does not have to be wrapped in<Host>.
Files:
src/components/slider/slider.tsx
⚙️ CodeRabbit Configuration File
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
Files:
src/components/slider/slider.tsx
**/*.{ts,tsx}
⚙️ CodeRabbit Configuration File
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
Files:
src/components/slider/slider.tsx
src/components/**/*.tsx
⚙️ CodeRabbit Configuration File
src/components/**/*.tsx: When contributors add new props to existing components in the lime-elements repository, they should always add documentation examples that demonstrate the new prop's usage and explain how it works. This helps with user adoption, feature discoverability, and maintains comprehensive documentation.
Files:
src/components/slider/slider.tsx
🧠 Learnings (3)
📓 Common learnings
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3581
File: src/components/chip-set/chip-set.tsx:626-626
Timestamp: 2025-07-02T12:49:21.522Z
Learning: When contributors add new props to existing components in the lime-elements repository, they should always add documentation examples that demonstrate the new prop's usage and explain how it works. This helps with user adoption, feature discoverability, and maintains comprehensive documentation.
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3518
File: src/components/chip-set/chip-set.tsx:367-385
Timestamp: 2025-04-14T12:22:22.298Z
Learning: In limel-chip-set, the onClick handler should be placed on the content div (<div slot="content">) rather than on the limel-notched-outline component, and should only be applied when type is 'input'.
src/components/slider/slider.scss (12)
Learnt from: CR
PR: Lundalogik/lime-elements#0
File: .cursor/rules/css-class-names-in-components-using-shadow-dom.mdc:0-0
Timestamp: 2025-07-21T00:27:58.489Z
Learning: Applies to src/components/**/*.scss : Do not use BEM-style class names in CSS for components, as components use shadow-DOM.
Learnt from: Kiarokh
PR: #3567
File: src/components/ai-avatar/ai-avatar.scss:0-0
Timestamp: 2025-05-28T08:37:45.610Z
Learning: In SCSS files, CSS property values can be split across multiple lines when properly indented, particularly for complex values like animation shorthands with CSS custom properties. This is valid SCSS syntax that compiles correctly to CSS and is often the result of auto-formatting tools.
Learnt from: adrianschmidt
PR: #3529
File: src/components/ai-avatar/ai-avatar.scss:20-22
Timestamp: 2025-04-29T14:15:32.104Z
Learning: In components using Shadow DOM (like those in lime-elements), the universal selector * is already scoped to the component's shadow tree and won't cause unwanted side effects outside the component. There's no functional difference between * and :host * in this context.
Learnt from: Kiarokh
PR: #3624
File: src/components/button-group/button-group.scss:115-118
Timestamp: 2025-07-30T14:20:41.779Z
Learning: In lime-elements, CSS custom properties like --limel-theme-surface-background-color are applied at :root level via core-styles.scss and should be expected to be available without fallbacks. The library's architecture guarantees these theme variables are present, so adding fallbacks would be redundant and go against the design intention.
Learnt from: Kiarokh
PR: #3624
File: src/components/button-group/button-group.scss:115-118
Timestamp: 2025-07-30T14:20:41.779Z
Learning: In lime-elements, CSS custom properties like --limel-theme-surface-background-color are applied at :root level via core-styles.scss and should be expected to be available without fallbacks. The library's architecture guarantees these theme variables are present, so adding fallbacks would be redundant and go against the design intention.
Learnt from: Kiarokh
PR: #3518
File: src/components/notched-outline/examples/notched-outline-basic.scss:11-22
Timestamp: 2025-04-14T10:01:18.721Z
Learning: Examples in Lime Elements use Shadow DOM (with shadow: true in the component decorator) for style encapsulation, making additional class-based selectors unnecessary. This ensures styles are automatically scoped to each component without leaking to the rest of the application.
Learnt from: Kiarokh
PR: #3529
File: src/components/lime-ai-avatar/lime-ai-avatar.scss:24-28
Timestamp: 2025-04-25T14:22:02.157Z
Learning: In lime-elements, CSS custom properties follow a specific naming convention:
- Internal use variables (not for consumers) are prefixed with
--limel-component-name- - External use variables (intended for consumers) are prefixed with
--component-name-
Variables documented with JSDoc comments are typically meant for external use.
Learnt from: Kiarokh
PR: #3529
File: src/components/lime-ai-avatar/lime-ai-avatar.scss:96-122
Timestamp: 2025-04-25T14:18:07.755Z
Learning: In Lime Elements components that use Shadow DOM (with shadow: true in the component decorator), keyframe animation names don't need to be namespaced as they are automatically scoped to the component and won't collide with animations from other components.
Learnt from: Kiarokh
PR: #3529
File: src/components/lime-ai-avatar/lime-ai-avatar.scss:1-3
Timestamp: 2025-04-25T14:23:56.018Z
Learning: In the lime-elements repository, CSS custom properties prefixed with --lime- are considered internal implementation details and should not be documented in JSDoc comments, while other custom properties should be documented as they are part of the public API.
Learnt from: Kiarokh
PR: #3518
File: src/components/chip-set/chip-set.tsx:367-385
Timestamp: 2025-04-14T12:22:22.298Z
Learning: In limel-chip-set, the onClick handler should be placed on the content div (
Learnt from: adrianschmidt
PR: #3529
File: src/components/lime-ai-avatar/lime-ai-avatar.tsx:30-38
Timestamp: 2025-04-25T13:32:56.396Z
Learning: For StencilJS components in lime-elements, prefer using the <Host> element to wrap child elements in the render method instead of returning an array of elements. This follows the standard pattern in the codebase and eliminates the need for key attributes.
Learnt from: adrianschmidt
PR: #3529
File: src/components/lime-ai-avatar/lime-ai-avatar.tsx:30-38
Timestamp: 2025-04-25T13:32:56.396Z
Learning: For StencilJS components in lime-elements, prefer using the <Host> element to wrap child elements in the render method instead of returning an array of elements, which eliminates the need for key attributes and follows StencilJS best practices.
src/components/slider/slider.tsx (4)
Learnt from: Kiarokh
PR: #3581
File: src/components/chip-set/chip-set.tsx:626-626
Timestamp: 2025-07-02T12:49:21.522Z
Learning: When contributors add new props to existing components in the lime-elements repository, they should always add documentation examples that demonstrate the new prop's usage and explain how it works. This helps with user adoption, feature discoverability, and maintains comprehensive documentation.
Learnt from: adrianschmidt
PR: #3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-17T09:39:36.254Z
Learning: For lime-elements, example files should import types from the public API using '@limetech/lime-elements' rather than defining duplicate types locally. This includes types like EditorUiType which are part of the component's public API.
Learnt from: Kiarokh
PR: #3518
File: src/components/chip-set/chip-set.tsx:367-385
Timestamp: 2025-04-14T12:22:22.298Z
Learning: In limel-chip-set, the onClick handler should be placed on the content div (
Learnt from: adrianschmidt
PR: #3529
File: src/components/lime-ai-avatar/lime-ai-avatar.tsx:30-38
Timestamp: 2025-04-25T13:32:56.396Z
Learning: For StencilJS components in lime-elements, prefer using the <Host> element to wrap child elements in the render method instead of returning an array of elements. This follows the standard pattern in the codebase and eliminates the need for key attributes.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
🔇 Additional comments (7)
src/components/slider/slider.scss (5)
4-4: LGTM: Proper mixins import addedThe mixins import is correctly added to support the helper line mixin used later in the file.
13-15: LGTM: Appropriate layout changes for new structureThe flex column layout with isolation and relative positioning properly supports the new
limel-notched-outlinewrapper structure.
17-22: LGTM: Conditional styling for invalid stateThe conditional styling correctly hides the notched outline border and background when the slider is not marked as invalid, which aligns with the new
invalidproperty functionality.
24-24: LGTM: Proper use of helper line mixinThe mixin correctly implements the conditional hiding of helper lines when not needed, following established patterns in the codebase.
26-31: LGTM: Content slot styling updated appropriatelyThe content slot styling properly accommodates the new slot structure with full width, flex column layout, and appropriate padding for the slider content.
src/components/slider/slider.tsx (2)
165-226: LGTM: Well-structured refactor with notched outline wrapperThe refactoring to use
limel-notched-outlinewrapper is well-implemented:
- All necessary props are correctly passed to the wrapper
- Content is properly slotted within the
contentslot- The slider structure and functionality are preserved
- Accessibility attributes are maintained (aria-labelledby, aria-controls)
The use of
<Host>wrapper follows StencilJS best practices for the component.
272-272: LGTM: Helper line properly receives invalid stateThe helper line component correctly receives the
invalidproperty, ensuring consistent validation styling throughout the component.
src/components/slider/slider.scss
Outdated
| flex-direction: column; | ||
| } | ||
|
|
||
| :host(limel-slider:not([invalid]):not([invalid='true'])) { |
There was a problem hiding this comment.
| :host(limel-slider:not([invalid]):not([invalid='true'])) { | |
| :host(limel-slider:not([invalid]):not([invalid='true'])), | |
| // We don't want the gray notched outlines around the slider by default | |
| :host(limel-slider:[disabled]):not([disabled='false']) { | |
| // and we don't want the `disabled` but `invalid` slider to show any | |
| // red lines. Since users cannot do anything to fix a disabled but invalid slider | |
LucyChyzhova
left a comment
LucyChyzhova
left a comment
There was a problem hiding this comment.
Really nice! I love the refactoring part as well!
Note that for simplicity, the required prop does not have any visual representation yet. The required asterisk is automatically added in the next commit.
Kiarokh
force-pushed
the
notched-slider
branch
2 times, most recently
from
754220c to
2e4446d
Compare
The main render method is split and different parts of the user interface have now distinct render methods, with single responsibility.
civing
left a comment
civing
left a comment
There was a problem hiding this comment.
I've only looked at the API change
|
🎉 This PR is included in version 38.21.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
continuation of #3518
Summary by CodeRabbit
New Features
Style
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: