fix: panel styles and classNames not working #409
Conversation
|
@keiseiTi is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
|
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. Walkthrough将 Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as useItems / convertItemsToNodes
participant Item as items[i]
participant Merge as MergeProps
participant Panel as CollapsePanel
Note over Hook,Item: 遍历每个 item，准备渲染 props
Hook->>Item: 读取 item.props（含可选 classNames/styles）
Hook->>Merge: 提供 collapseClassNames/collapseStyles（全局）
Item->>Merge: 提供 item.classNames/item.styles（按项）
Merge-->>Hook: 返回 mergeClassNames / mergeStyles
Hook->>Panel: 渲染 CollapsePanel（使用合并后的 classNames/styles）
Panel-->>Hook: 渲染节点返回
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 分钟
Possibly related PRs
Suggested reviewers
诗
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
Summary of ChangesHello @keiseiTi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where custom Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue where styles and classNames on individual panels were not being applied. The changes correctly address this for panels provided via the items prop by prioritizing panel-specific properties over the ones from the parent Collapse component. However, as noted in my comment, the fix is incomplete because it doesn't cover panels passed as children. I've recommended extending the fix to the getNewChild function to ensure consistent behavior across the component.
| expandIcon, | ||
| classNames: collapseClassNames, | ||
| styles, | ||
| styles: collapseStyles, |
There was a problem hiding this comment.
Renaming styles to collapseStyles is a good step to avoid naming collisions. However, this fix for panel-specific styles and classNames appears to be incomplete because it only applies to panels defined via the items prop.
The getNewChild function (which handles Panel components passed as children, starting on line 88) has not been updated. It still passes the Collapse component's styles and classNames to each CollapsePanel, ignoring the props on the Panel itself.
To ensure consistent behavior, I recommend applying a similar fix to the getNewChild function. This would involve:
- Destructuring
classNamesandstylesfromchild.props. - Passing
childClassNames || collapseClassNamesandchildStyles || collapseStylesto the cloned element.
This will make the fix comprehensive, even though getNewChild is deprecated.
![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
🧹 Nitpick comments (1)
src/hooks/useItems.tsx (1)
36-37: item 级 classNames/styles 覆盖逻辑正确，可考虑后续扩展为“部分合并”现在从
item解构出classNames、styles，并在渲染时使用：
- 当 item 提供
classNames/styles时，优先使用面板自己的配置；- 当未提供时，回退到
collapseClassNames/collapseStyles；这正好修复了“面板级 classNames/styles 不生效”的问题，也与 PR 描述“有面板级就用面板级，否则用 collapse 级”一致。当前实现是“全量替换”，即一旦 item 传入对象，就完全覆盖 collapse 级默认值。
如果未来有需求在 item 上只覆盖部分 key（例如只改 header 的样式，但保留 collapse 默认的其他部位样式），可以考虑改为在这里做一次对象合并（例如先用 collapse 默认，再叠加 item 的配置），以支持更细粒度的定制。
Also applies to: 65-66
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/hooks/useItems.tsx(3 hunks)
🔇 Additional comments (1)
src/hooks/useItems.tsx (1)
24-26: 为 Collapse 级别样式使用别名是合理且必要的调整这里将 props 中的
classNames/styles解构为collapseClassNames/collapseStyles，避免后续与 item 自身的classNames/styles命名冲突，同时也让“容器默认值 vs 面板局部配置”的职责更加清晰，逻辑上没有问题。
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #409 +/- ##
=======================================
Coverage 99.16% 99.16%
=======================================
Files 5 5
Lines 120 120
Branches 45 44 -1
=======================================
Hits 119 119
Misses 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/hooks/useItems.tsx
Outdated
| {...restProps} | ||
| classNames={collapseClassNames} | ||
| styles={styles} | ||
| classNames={classNames || collapseClassNames} |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/hooks/useItems.tsx (1)
156-162:getNewChild函数未应用相同的合并逻辑。虽然此函数已标记为 deprecated，但在通过
children方式传入 Panel 时仍会被使用。当前实现直接传递collapseClassNames和styles，未与child.props中的classNames和styles合并。
🧹 Nitpick comments (1)
tests/index.spec.tsx (1)
902-954: 测试用例验证了合并行为，建议补充title和icon的测试。测试正确验证了：
- collapse 级别和 panel 级别的 classNames 同时生效（第 944-947 行）
- panel 级别的 styles 可以覆盖 collapse 级别的值（第 950-953 行）
当
useItems.tsx中补全title和icon的合并逻辑后，建议扩展此测试以覆盖这两个字段。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/hooks/useItems.tsx(4 hunks)tests/index.spec.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/index.spec.tsx (1)
src/index.tsx (1)
Collapse(10-10)
src/hooks/useItems.tsx (1)
src/interface.ts (1)
SemanticName(44-44)
🔇 Additional comments (1)
src/hooks/useItems.tsx (1)
84-85: 合并后的 classNames 和 styles 正确传递给 CollapsePanel。实现思路正确，通过
clsx合并 className 字符串，通过对象展开合并 styles，使得 panel 级别的配置可以覆盖或扩展 collapse 级别的配置。
| const mergeClassNames: Partial<Record<SemanticName, string>> = { | ||
| ...collapseClassNames, | ||
| header: clsx(collapseClassNames?.header, classNames?.header), | ||
| body: clsx(collapseClassNames?.body, classNames?.body), | ||
| }; | ||
|
|
||
| const mergeStyles: Partial<Record<SemanticName, React.CSSProperties>> = { | ||
| ...collapseStyles, | ||
| header: { | ||
| ...collapseStyles?.header, | ||
| ...styles?.header, | ||
| }, | ||
| body: { | ||
| ...collapseStyles?.body, | ||
| ...styles?.body, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
合并逻辑缺少 title 和 icon 的处理。
SemanticName 类型包含 'header' | 'title' | 'body' | 'icon' 四个值，但当前合并逻辑仅显式处理了 header 和 body。如果 panel 级别设置了 classNames.title、classNames.icon、styles.title 或 styles.icon，这些值不会与 collapse 级别的值合并。
建议补全 title 和 icon 的合并逻辑：
const mergeClassNames: Partial<Record<SemanticName, string>> = {
...collapseClassNames,
header: clsx(collapseClassNames?.header, classNames?.header),
body: clsx(collapseClassNames?.body, classNames?.body),
+ title: clsx(collapseClassNames?.title, classNames?.title),
+ icon: clsx(collapseClassNames?.icon, classNames?.icon),
};
const mergeStyles: Partial<Record<SemanticName, React.CSSProperties>> = {
...collapseStyles,
header: {
...collapseStyles?.header,
...styles?.header,
},
body: {
...collapseStyles?.body,
...styles?.body,
},
+ title: {
+ ...collapseStyles?.title,
+ ...styles?.title,
+ },
+ icon: {
+ ...collapseStyles?.icon,
+ ...styles?.icon,
+ },
};📝 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 mergeClassNames: Partial<Record<SemanticName, string>> = { | |
| ...collapseClassNames, | |
| header: clsx(collapseClassNames?.header, classNames?.header), | |
| body: clsx(collapseClassNames?.body, classNames?.body), | |
| }; | |
| const mergeStyles: Partial<Record<SemanticName, React.CSSProperties>> = { | |
| ...collapseStyles, | |
| header: { | |
| ...collapseStyles?.header, | |
| ...styles?.header, | |
| }, | |
| body: { | |
| ...collapseStyles?.body, | |
| ...styles?.body, | |
| }, | |
| }; | |
| const mergeClassNames: Partial<Record<SemanticName, string>> = { | |
| ...collapseClassNames, | |
| header: clsx(collapseClassNames?.header, classNames?.header), | |
| body: clsx(collapseClassNames?.body, classNames?.body), | |
| title: clsx(collapseClassNames?.title, classNames?.title), | |
| icon: clsx(collapseClassNames?.icon, classNames?.icon), | |
| }; | |
| const mergeStyles: Partial<Record<SemanticName, React.CSSProperties>> = { | |
| ...collapseStyles, | |
| header: { | |
| ...collapseStyles?.header, | |
| ...styles?.header, | |
| }, | |
| body: { | |
| ...collapseStyles?.body, | |
| ...styles?.body, | |
| }, | |
| title: { | |
| ...collapseStyles?.title, | |
| ...styles?.title, | |
| }, | |
| icon: { | |
| ...collapseStyles?.icon, | |
| ...styles?.icon, | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In src/hooks/useItems.tsx around lines 63 to 79, the merge logic only handles
'header' and 'body' but misses 'title' and 'icon', so panel-level
classNames.title/icon and styles.title/icon are not merged with collapse-level
values; update mergeClassNames to include title: clsx(collapseClassNames?.title,
classNames?.title) and icon: clsx(collapseClassNames?.icon, classNames?.icon),
and update mergeStyles to include title: { ...collapseStyles?.title,
...styles?.title } and icon: { ...collapseStyles?.icon, ...styles?.icon } so all
four SemanticName entries are consistently merged.
| isActive = activeKey.indexOf(key) > -1; | ||
| } | ||
|
|
||
| const mergeClassNames: Partial<Record<SemanticName, string>> = { |
There was a problem hiding this comment.
我建议是在 Collapse 顶层做一个 SemanticContext，然后传入 classNames 和 styles，然后在 panel 里读取 className 和 style 以及 contextClassName 和 contextStyle，在消费的时候做聚合。
现在合并在 useItems 这层，如果未来额外加 semantic structure 很容易忘记。
| expect(bodyElement.classList).toContain('custom-body-panel'); | ||
|
|
||
| // check styles | ||
| expect(headerElement.style.color).toBe('blue'); |
There was a problem hiding this comment.
可以直接 expect(xx).toHaveStyle({ xxx: xxx })
2 participants
If the panel is set to styles or classNames, use it's properties, otherwise use collpase's styles or classNames.
Summary by CodeRabbit
新功能
改进
测试
注意
✏️ Tip: You can customize this high-level summary in your review settings.