-
-
Notifications
You must be signed in to change notification settings - Fork 198
refactor: remove useless structure #502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Warning Rate limit exceeded@zombieJ has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 59 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (8)
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本次变更统一了 Tooltip/Popup 的语义化样式接口与 DOM 结构:Popup 将 inner 容器合并为单层 body;Tooltip 引入基于语义键(root/arrow/body)的 classNames 与 styles,调整箭头配置为标准化对象;测试同步更新选择器与断言,类型导入方式调整。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App
participant Tooltip as Tooltip (render)
participant Arrow as getArrowConfig
participant Trigger as Trigger
participant Popup as Popup
App->>Tooltip: render(props: { classNames, styles, showArrow, ... })
Tooltip->>Arrow: normalize(showArrow, classNames.arrow, arrowContent)
Arrow-->>Tooltip: { visible, className, content }
Tooltip->>Trigger: render({ arrow: ArrowConfig, popup: <Popup .../> })
Trigger->>Popup: render({ overlayInnerStyle, className: body/root })
Popup-->>Trigger: single-layer body div
Trigger-->>App: mounted tooltip
sequenceDiagram
autonumber
participant Old as 旧版 Popup
participant New as 新版 Popup
Note over Old: 双层结构<br/>outer(content) -> inner
Old->>Old: style 来自 innerStyle
Old-->>Old: className: ${prefixCls}-content
Note over New: 单层结构<br/>outer(body)
New->>New: style: { ...overlayInnerStyle, ...style }
New-->>New: className: ${prefixCls}-body (+className/+bodyClassName)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @zombieJ, 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!
此拉取请求重构了 Popup 和 Tooltip 组件,以简化其 DOM 结构并引入更具语义化和一致性的样式和类名应用方式。这包括减少 Popup 中的 DOM 嵌套,并在 Tooltip 中提供新的 classNames 和 styles 属性,这些属性接受一个将语义名称(root、arrow、body)映射到其各自样式/类名的对象,同时废弃了旧的、粒度较差的样式属性。
Highlights
- DOM 结构简化: Popup 组件的内部 DOM 层级被扁平化,将两层 div 结构简化为单层 div,并调整了类名和样式属性的处理方式。
- 语义化样式 API: Tooltip 组件现在提供了新的 classNames 和 styles 属性,允许通过结构化对象将样式和类名应用于工具提示的不同语义部分(根、箭头、主体)。
- 旧样式属性的废弃: 几个旧的样式属性,如 overlayStyle、overlayClassName 和 overlayInnerStyle 已被废弃,转而使用新的语义化样式 API。
- 标准化箭头配置: Tooltip 中的 arrow 属性现在使用统一的配置对象,允许将语义化类名应用于箭头组件。
- 测试更新: 相应的测试已更新,以反映新的 DOM 结构、类名和断言样式,确保代码变更的正确性。
Using Gemini Code Assist
The 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 /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands on the current page.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
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 .gemini/
folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #502 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 33 37 +4
Branches 12 15 +3
=========================================
+ Hits 33 37 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
你好,感谢你的贡献。这次的重构很好地简化了 Popup
组件的 DOM 结构,并为 Tooltip
引入了更具语义化的样式 API (classNames
和 styles
),这使得组件更易于定制和维护。相关的测试用例也已正确更新。
我只发现了一个小问题,在 getArrowConfig
函数中,对 arrowContent
的处理逻辑在某些边缘情况下可能不符合预期。除此之外,代码质量很高。请查看我的具体评论。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Tooltip.tsx (1)
26-39
: 为废弃属性增加开发期告警,平滑迁移。建议在开发环境检测到 overlayStyle/overlayClassName/overlayInnerStyle 时输出一次性 warning,引导用户迁移到 classNames/styles。
可在组件顶层加轻量告警(仅开发环境、仅第一次)以减少破坏性升级的支持成本。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Popup.tsx
(1 hunks)src/Tooltip.tsx
(5 hunks)tests/index.test.tsx
(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: thinkasany
PR: react-component/tooltip#484
File: src/Popup.tsx:20-25
Timestamp: 2024-11-28T13:00:19.052Z
Learning: `overlayInnerClassName` 属性在 `Popup.tsx` 中是供内部使用的,已经通过 `classNames.inner` 在现有的测试用例中得到验证,无需单独添加测试。
📚 Learning: 2024-11-28T13:00:19.052Z
Learnt from: thinkasany
PR: react-component/tooltip#484
File: src/Popup.tsx:20-25
Timestamp: 2024-11-28T13:00:19.052Z
Learning: `overlayInnerClassName` 属性在 `Popup.tsx` 中是供内部使用的,已经通过 `classNames.inner` 在现有的测试用例中得到验证,无需单独添加测试。
Applied to files:
src/Popup.tsx
src/Tooltip.tsx
🔇 Additional comments (8)
tests/index.test.tsx (3)
3-3
: 类型导入写法正确,避免运行时引入与循环依赖。LGTM。
76-78
: 断言目标切换到 .rc-tooltip-body 合理。与 Popup 合并 DOM 层级后的结构一致;此处读取的是行内样式,稳定不依赖 computedStyle。
271-277
: 确认在 Jest 测试环境中注入 @testing-library/jest-dom(setupFilesAfterEnv)仓库内未找到对 @testing-library/jest-dom 的导入或任何 setupFilesAfterEnv 配置;若未注入,toHaveClass 等 jest-dom 断言会失败。
- 在测试初始化文件(例如 setupTests.ts / setupTests.js)中添加:import '@testing-library/jest-dom';
- 在 package.json 或 jest.config.js/ts 中确保有 setupFilesAfterEnv 指向该初始化文件,例如 "setupFilesAfterEnv": ["/test/setupTests.ts"]。
src/Popup.tsx (1)
15-25
: 简化 DOM 层级与语义命名到 -body,合并样式优先级正确。style={{ ...overlayInnerStyle, ...style }} 保证新 props(styles.body) 覆盖旧 props(overlayInnerStyle)。同时保留 role="tooltip" 与 id 供 aria-describedby 使用。实现到位。
请同步确认样式表中对原 .rc-tooltip-content 的选择器已迁移到 .rc-tooltip-body,避免样式漂移。
src/Tooltip.tsx (4)
11-12
: 引入 SemanticName 语义键清晰,便于统一样式映射。
112-114
: body 样式透传方式正确。Popup 端已按 overlayInnerStyle 先、style 后合并;此处传 styles.body 能覆盖旧内联样式,符合预期。
165-165
: arrow={getArrowConfig()} 对接一致。在修正 getArrowConfig 的返回类型后,该处类型/运行时行为均匹配 rc-trigger 预期。
129-145
: 修正 TypeScript 收窄问题:为 getArrowConfig 标注并将 showArrow 归一化为 ArrowTypeshowArrow === true ? {} : showArrow 会推断为 {} | ArrowType,随后访问 className/content 在严格模式下可能报错;建议标注返回类型并把 true 分支断言为 ArrowType。
文件:src/Tooltip.tsx 行:129-145
- // Process arrow configuration - const getArrowConfig = () => { + // Process arrow configuration + const getArrowConfig = (): ArrowType | false => { if (!showArrow) { return false; } - - // Convert true to object for unified processing - const arrowConfig = showArrow === true ? {} : showArrow; - - // Apply semantic styles with unified logic - return { - ...arrowConfig, - className: classNames(arrowConfig.className, tooltipClassNames?.arrow), - content: arrowConfig.content || arrowContent, - }; + // Normalize to ArrowType for safe property access + const base: ArrowType = showArrow === true ? {} : (showArrow as ArrowType); + return { + ...base, + className: classNames(base.className, tooltipClassNames?.arrow), + content: base.content ?? arrowContent, + }; };另外请确认 ArrowType 是否包含可选的 content 字段;若无,请扩展类型或避免返回该字段。
按照新的语义化结构裁剪 dom 层级
Summary by CodeRabbit