-
-
Notifications
You must be signed in to change notification settings - Fork 589
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
fix: The fixed attribute does not take effect #1193
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Walkthrough此次更改涉及 Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
搞个测试用例 cover 一下 |
ok,稍等 |
看了下测试用例,没考虑到expandIconColumnIndex小于0时是不渲染的,修改了一下判断条件 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1193 +/- ##
=======================================
Coverage 97.75% 97.75%
=======================================
Files 75 75
Lines 7484 7404 -80
Branches 1121 1124 +3
=======================================
- Hits 7316 7238 -78
+ Misses 162 160 -2
Partials 6 6 ☔ View full report in Codecov by Sentry. |
佬,这个pr为啥还不给合并捏 |
src/hooks/useColumns/index.tsx
Outdated
@@ -176,9 +176,13 @@ function useColumns<RecordType>( | |||
// >>> Insert expand column if not exist | |||
if (!cloneColumns.includes(EXPAND_COLUMN)) { | |||
const expandColIndex = expandIconColumnIndex || 0; | |||
if (expandColIndex >= 0) { | |||
console.log(expandColIndex); |
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.
删除调试语句
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.
ok
最好还是单独给个用例,单看 snap 的改变不容易看出是不是符合预期。 |
哦哦,明白鸟 |
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
🧹 Outside diff range and nitpick comments (2)
src/hooks/useColumns/index.tsx (1)
247-247
: 建议完善 useEffect 依赖项的处理当前代码中存在多处 eslint-disable-next-line react-hooks/exhaustive-deps 注释。建议:
- 添加注释说明为什么排除这些依赖项
- 评估是否所有被排除的依赖项确实不需要包含在依赖数组中
建议添加类似如下的注释:
// eslint-disable-next-line react-hooks/exhaustive-deps + // 仅在列结构发生实质性变化时才需要重新计算,忽略其他依赖项以避免不必要的重新渲染
Also applies to: 266-266, 275-275
tests/ExpandRow.spec.jsx (1)
233-261
: 测试用例实现完整且合理!测试用例很好地覆盖了固定列在可展开行中的基本功能:
- 验证了
fixed: 'left'
的场景- 验证了
fixed: 'right'
的场景- 通过检查 CSS 类名确保了正确的渲染行为
建议增加以下测试场景以提高覆盖率:
- 验证嵌套表格中的固定列行为
- 测试动态切换 fixed 属性值的情况
- 添加边界情况的测试,如空数据时的表现
示例代码:
it('should handle fixed columns in nested expandable rows', () => { const nestedData = [ { key: 0, name: 'Parent', children: [{ key: 1, name: 'Child' }] } ]; const wrapper = mount( createTable({ columns, data: nestedData, scroll: { x: 903 }, expandable: { expandedRowRender, fixed: 'left' } }) ); expect(wrapper.find('.rc-table-has-fix-left').length).toBe(1); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tests/__snapshots__/ExpandRow.spec.jsx.snap
is excluded by!**/*.snap
📒 Files selected for processing (2)
- src/hooks/useColumns/index.tsx (5 hunks)
- tests/ExpandRow.spec.jsx (1 hunks)
🔇 Additional comments (2)
src/hooks/useColumns/index.tsx (2)
203-204
: 固定列处理逻辑优化合理代码变更使固定列的处理更加直观:
- 优先使用传入的 fixed 属性
- 仅在未指定 fixed 时才参考前一列的状态
这样的改动提高了代码的可预测性和可维护性。
179-184
: 展开列插入逻辑的改进值得肯定新的插入逻辑更好地处理了固定列的场景,特别是:
- 左固定和非固定列的处理更加明确
- 为右固定列添加了专门的处理逻辑
建议执行以下脚本验证改动的完整性:
✅ Verification successful
展开列和固定列功能的测试覆盖情况良好
测试用例已经完整覆盖了所有关键场景:
- 左固定和右固定列的展开功能
- 不同的 expandIconColumnIndex 配置
- 固定列与展开列的组合使用
- EXPAND_COLUMN 的重复检查和位置验证
代码改动与测试用例保持一致,验证通过。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # 描述:验证展开列在不同固定列场景下的使用情况 # 预期:应该能找到测试用例覆盖所有固定列场景 # 搜索与展开列相关的测试用例 rg -l "EXPAND_COLUMN.*fixed" tests/Length of output: 37
Script:
#!/bin/bash # 搜索展开列和固定列相关的测试用例,使用更宽泛的搜索模式 rg -l "expandable.*fixed|fixed.*expand" tests/ # 搜索展开列相关的测试文件 fd "expand.*test" tests/ # 查找包含 EXPAND_COLUMN 常量的测试文件 rg -l "EXPAND_COLUMN" tests/ # 查找表格组件的测试文件 fd "table.*test" tests/Length of output: 201
Script:
#!/bin/bash # 检查展开行测试文件中的固定列测试用例 rg "fixed.*expand|expand.*fixed" -A 10 -B 10 tests/ExpandRow.spec.jsx # 检查表格测试文件中的相关测试用例 rg "fixed.*expand|expand.*fixed" -A 10 -B 10 tests/Table.spec.jsx # 查看 EXPAND_COLUMN 的具体使用场景 rg "EXPAND_COLUMN" -A 5 -B 5 tests/ExpandRow.spec.jsx tests/Table.spec.jsxLength of output: 8237
修复表格expandable中设置fixed:right不生效,issues:ant-design/ant-design#50846
对于fixed的处理不正确,一个是判断逻辑有问题,fixed不应该和expandIconColumnIndex一起判断,一个是插入扩展列的时候没有按照fixed的值来处理
Summary by CodeRabbit