feat(select): enable showing a custom component along with options#4068
feat(select): enable showing a custom component along with options#4068
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds optional ChangesPrimary Component Feature for limel-select
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4068/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/select/select.tsx (1)
385-409:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
getFirstNativeAutoSelectOptionfires spuriously on mobile whenprimaryComponentis presentThe new
hasPrimaryComponent()guard correctly prevents native<select>rendering, butgetFirstNativeAutoSelectOptionis only gated on!this.isMobileDeviceandthis.multiple. On a mobile device whose options include aprimaryComponent,nativeisfalse(custom dropdown shown), yetgetFirstNativeAutoSelectOptionstill returns the first option, causingopenMenuto emit an unwantedchangeevent before the user makes any selection.🐛 Proposed fix
private getFirstNativeAutoSelectOption(): Option | undefined { - if (this.hasChanged || !this.isMobileDevice || this.multiple) { + if (this.hasChanged || !this.isMobileDevice || this.multiple || this.hasPrimaryComponent()) { return undefined; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/select/select.tsx` around lines 385 - 409, getFirstNativeAutoSelectOption currently runs on mobile even when a primaryComponent is present, causing an unwanted auto-selection; update getFirstNativeAutoSelectOption to also return undefined if hasPrimaryComponent() is true (in addition to the existing guards this.isMobileDevice and this.multiple) so it won’t pick the first option when the component is rendering a custom dropdown; check the start of getFirstNativeAutoSelectOption (the early-return conditions) and add the hasPrimaryComponent() guard before computing options.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/select/select.tsx`:
- Around line 385-409: getFirstNativeAutoSelectOption currently runs on mobile
even when a primaryComponent is present, causing an unwanted auto-selection;
update getFirstNativeAutoSelectOption to also return undefined if
hasPrimaryComponent() is true (in addition to the existing guards
this.isMobileDevice and this.multiple) so it won’t pick the first option when
the component is rendering a custom dropdown; check the start of
getFirstNativeAutoSelectOption (the early-return conditions) and add the
hasPrimaryComponent() guard before computing options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb4885b8-3e8a-4023-9a41-48c8e120dfd3
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (5)
src/components/select/examples/select-with-primary-component.tsxsrc/components/select/option.types.tssrc/components/select/select.scsssrc/components/select/select.template.tsxsrc/components/select/select.tsx
Kiarokh
force-pushed
the
select-option-component
branch
from
4c741de to
3a31537
Compare
Kiarokh
force-pushed
the
select-option-component
branch
from
3a31537 to
2ef910e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/select/examples/select-with-primary-component.tsx`:
- Line 29: Mark the instance properties as readonly: change the declaration of
the options property to "private readonly options: Option[] = [...]" and the
class property handleChange (the arrow function assigned to handleChange) to
"private readonly handleChange = (...) => { ... }" so neither can be reassigned;
update both occurrences (around the options definition and the handleChange
property) to suppress SonarCloud warnings and communicate intent.
In `@src/util/render-list-component.tsx`:
- Around line 4-15: The JSDoc for renderListComponent is missing a `@param` for
the component parameter; update the block above the renderListComponent function
to include a `@param` component {ListComponent | undefined} with a short
description (e.g., "the ListComponent to render, or undefined") so the
jsdoc/require-param lint rule passes while keeping the existing description of
return behavior intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ddc2e013-bd11-44a9-a23b-fd093c1aef05
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (8)
src/components/list-item/list-item.tsxsrc/components/select/examples/select-with-primary-component.tsxsrc/components/select/option.types.tssrc/components/select/select.e2e.tsxsrc/components/select/select.scsssrc/components/select/select.template.tsxsrc/components/select/select.tsxsrc/util/render-list-component.tsx
Kiarokh
force-pushed
the
select-option-component
branch
2 times, most recently
from
5cae4a6 to
e334ce4
Compare
The `primaryComponent` slot rendered any string passed in `name` as a JSX tag. With consumer-supplied props spread on the result, a malicious or untrusted `name` like `iframe`, `script`, or `a` could be rendered with arbitrary attributes. Reject names that don't contain a hyphen, matching the HTML custom element spec. Custom elements always contain a hyphen, so legitimate consumers are unaffected.
Move the `ListComponent` rendering logic, including the hyphen check introduced in the previous commit, out of `list-item.tsx` and into `src/util/render-list-component.tsx`. This allows other components that render the same data shape (such as `limel-select`) to share the exact same rendering and validation behavior, so future changes to how `ListComponent` is interpreted only need to land in one place. No behavior change.
Adds a `primaryComponent` field to `Option` so consumers can render a custom component along with the icon and text of an option. The component is rendered both in the dropdown list (forwarded to `limel-list-item`) and in the trigger area when the option is selected. When any option carries a `primaryComponent`, the native mobile dropdown is bypassed in favor of the menu-based dropdown, since native `<option>` elements cannot host custom components. Both render paths go through the shared `renderListComponent` helper, so the hyphen-based safety check on the component name applies here too.
Kiarokh
force-pushed
the
select-option-component
branch
from
e334ce4 to
d185bc6
Compare
|
🎉 This PR is included in version 39.23.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
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: