feat(menu): use primaryComponent from the limel-list#3994
feat(menu): use primaryComponent from the limel-list#3994LucyChyzhova wants to merge 1 commit intomainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 13 minutes and 14 seconds. ⌛ 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 (7)
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as MenuListRenderer
participant Meta as MenuItemMeta
participant Primary as PrimaryComponent
participant User as User (DOM / Event)
Renderer->>Meta: render with props (commandText, badge, hotkey, disabled, primaryComponent?)
Meta->>Meta: renderPrimaryComponent() checks primaryComponent?.name
alt primaryComponent present
Meta->>Primary: instantiate PrimaryComponent + props + disabled={disabled}
Primary-->>Meta: rendered component node
end
Meta->>User: render commandText, badge, chevron after primary component
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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-3994/ |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/menu/menu.types.ts`:
- Around line 125-130: Add a documentation example file demonstrating how to use
the new primaryComponent prop for the Menu component (similar to the existing
ListItem example); specifically, create a new example (e.g.,
menu-primary-component.tsx) that shows Menu/MenuItem rendering an icon or custom
component via the primaryComponent prop, mirror patterns from
list-item-primary-component.tsx, export/register the example per the project's
examples convention, and ensure the JSDoc for primaryComponent in menu.types.ts
references this new example so developers can see usage with MenuItem and custom
ListComponent implementations.
🪄 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: da042012-9ecb-4aaf-b3aa-cddebb0490b3
📒 Files selected for processing (3)
src/components/list-item/menu-item-meta/menu-item-meta.tsxsrc/components/menu-list/menu-list-renderer.tsxsrc/components/menu/menu.types.ts
| /** | ||
| * Component used to render additional content in the menu item. | ||
| * Rendered before the item's text, similar to `primaryComponent` | ||
| * on `ListItem`. | ||
| */ | ||
| primaryComponent?: ListComponent; |
There was a problem hiding this comment.
I think that we maybe should add an example as mr Robot here says, what do you think?
| /** | ||
| * Component used to render additional content in the menu item. | ||
| * Rendered before the item's text, similar to `primaryComponent` | ||
| * on `ListItem`. | ||
| */ | ||
| primaryComponent?: ListComponent; |
There was a problem hiding this comment.
I think that we maybe should add an example as mr Robot here says, what do you think?
LucyChyzhova
force-pushed
the
feat/menu-list-reflect-primaryComponent
branch
from
5a80fc8 to
3cd9a50
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/menu/examples/menu-primary-component.tsx`:
- Line 2: The render method currently returns an array literal; update it to
import and use Host from '@stencil/core' and wrap the multiple child elements
inside <Host> instead of returning an array (modify the existing import line to
include Host and change the render implementation in the component class's
render method to return a single <Host> element containing the children). Ensure
no array is returned and that all previous child elements are placed as direct
children of Host.
In `@src/components/menu/menu.types.ts`:
- Around line 125-133: Update the JSDoc for the primaryComponent prop to clarify
that primaryComponent is rendered in the menu item's meta/trailing area (routed
via limel-menu-item-meta), is display-only and must remain non-interactive so
the row still behaves as a single ARIA menuitem; mention that it must not
contain focusable controls or assume leading/list-item placement, and update the
limel-example-menu-primary-component example text to reflect this contract.
🪄 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: 9832a307-083c-4c4c-b1c6-38221db80ee2
📒 Files selected for processing (7)
src/components/list-item/list-item.scsssrc/components/list-item/list-item.tsxsrc/components/list-item/menu-item-meta/menu-item-meta.tsxsrc/components/menu-list/menu-list-renderer.tsxsrc/components/menu/examples/menu-primary-component.tsxsrc/components/menu/menu.tsxsrc/components/menu/menu.types.ts
| @@ -0,0 +1,77 @@ | |||
| import { LimelMenuCustomEvent, MenuItem } from '@limetech/lime-elements'; | |||
| import { Component, h, State } from '@stencil/core'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -a "menu-primary-component.tsx" src/components/menu/examples/Repository: Lundalogik/lime-elements
Length of output: 142
🏁 Script executed:
# Check if file exists and examine the relevant lines
if [ -f "src/components/menu/examples/menu-primary-component.tsx" ]; then
echo "=== File Content with Line Numbers ==="
cat -n "src/components/menu/examples/menu-primary-component.tsx" | head -80
fiRepository: Lundalogik/lime-elements
Length of output: 2882
Wrap the example output in <Host>.
Returning an array literal from the render method violates the repo's StencilJS pattern. Use <Host> to wrap multiple child elements instead, which aligns with the codebase and removes the missing-key warnings.
♻️ Suggested change
-import { Component, h, State } from '@stencil/core';
+import { Component, h, Host, State } from '@stencil/core';
…
public render() {
- return [
- <limel-menu items={this.items} onSelect={this.handleSelect}>
- <limel-button label="Tasks" slot="trigger" />
- </limel-menu>,
- <limel-example-value value={this.lastSelectedItem} />,
- ];
+ return (
+ <Host>
+ <limel-menu items={this.items} onSelect={this.handleSelect}>
+ <limel-button label="Tasks" slot="trigger" />
+ </limel-menu>
+ <limel-example-value value={this.lastSelectedItem} />
+ </Host>
+ );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/menu/examples/menu-primary-component.tsx` at line 2, The
render method currently returns an array literal; update it to import and use
Host from '@stencil/core' and wrap the multiple child elements inside <Host>
instead of returning an array (modify the existing import line to include Host
and change the render implementation in the component class's render method to
return a single <Host> element containing the children). Ensure no array is
returned and that all previous child elements are placed as direct children of
Host.
| /** | ||
| * Component used to render additional content in the menu item. | ||
| * | ||
| * When the menu item is disabled, the `disabled` prop is automatically | ||
| * propagated to the primary component. | ||
| * | ||
| * See `limel-example-menu-primary-component` for usage. | ||
| */ | ||
| primaryComponent?: ListComponent; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clarify that menu primaryComponent is trailing, display-only content.
This public prop is routed through limel-menu-item-meta, while the row itself still behaves as a single menuitem. Please spell out here that it renders in the menu item's meta area and must stay non-interactive; otherwise consumers can assume list-item-like leading placement or nest focusable controls that break menu keyboarding. Please align the example copy with that contract too.
✏️ Suggested JSDoc update
/**
- * Component used to render additional content in the menu item.
+ * Component used to render additional trailing meta content in the menu item.
+ *
+ * It is rendered together with the menu item's meta content, before
+ * shortcut text, badges, and submenu chevrons.
+ *
+ * This content must be presentational only. Do not pass focusable or
+ * interactive components, since the row itself remains the single
+ * `menuitem`.
*
* When the menu item is disabled, the `disabled` prop is automatically
* propagated to the primary component.
*
* See `limel-example-menu-primary-component` for usage.
*/📝 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.
| /** | |
| * Component used to render additional content in the menu item. | |
| * | |
| * When the menu item is disabled, the `disabled` prop is automatically | |
| * propagated to the primary component. | |
| * | |
| * See `limel-example-menu-primary-component` for usage. | |
| */ | |
| primaryComponent?: ListComponent; | |
| /** | |
| * Component used to render additional trailing meta content in the menu item. | |
| * | |
| * It is rendered together with the menu item's meta content, before | |
| * shortcut text, badges, and submenu chevrons. | |
| * | |
| * This content must be presentational only. Do not pass focusable or | |
| * interactive components, since the row itself remains the single | |
| * `menuitem`. | |
| * | |
| * When the menu item is disabled, the `disabled` prop is automatically | |
| * propagated to the primary component. | |
| * | |
| * See `limel-example-menu-primary-component` for usage. | |
| */ | |
| primaryComponent?: ListComponent; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/menu/menu.types.ts` around lines 125 - 133, Update the JSDoc
for the primaryComponent prop to clarify that primaryComponent is rendered in
the menu item's meta/trailing area (routed via limel-menu-item-meta), is
display-only and must remain non-interactive so the row still behaves as a
single ARIA menuitem; mention that it must not contain focusable controls or
assume leading/list-item placement, and update the
limel-example-menu-primary-component example text to reflect this contract.
LucyChyzhova
force-pushed
the
feat/menu-list-reflect-primaryComponent
branch
from
3cd9a50 to
a63a9e6
Compare
| * | ||
| * See `limel-example-menu-primary-component` for usage. | ||
| */ | ||
| primaryComponent?: ListComponent; |
There was a problem hiding this comment.
I had a chat with @LucyChyzhova earlier today. Mayeb you have already talked about it.
But just to make sure you have heard my feedback 😄 :
I don't think it's a good idea to make the interface and APIs of the menu more complicated than it already is. The primary component slot is already being used by the menu itself, for other internal things (such as hotkeys, command text, caret, badge).
It's really hard to motivate having additional stuff here, decided by the consumer. They need to coexist and work well. The UI will be really hard to maintain if you add this, because we have to deal with many unforeseen scenarios. Please don't merge this PR. 🙏
if this was really a must have, a better choice would be perhaps introducing a new slot. But I wouldn't do that neither.
Regarding your design needs
For your particular use case, a badge would be enough, which already exists. No need for more features like this really, to address that need.
But I would also question that design. I think you have better choices for what you wanna achieve 🤗
4 participants
fix: https://github.com/Lundalogik/lime-crm-components/pull/4166
Summary by CodeRabbit
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: