-
-
Notifications
You must be signed in to change notification settings - Fork 618
Use Column width #1344
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
Use Column width #1344
Conversation
@crazyair is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
Caution Review failedFailed to post review comments. Configuration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2024-11-08T12:53:09.293Z
Applied to files:
🧬 Code graph analysis (2)docs/examples/scrollY.tsx (2)
docs/examples/stickyHeader.tsx (1)
Walkthrough本次变更移除了表格单元格的断词样式;为文档示例新增/完善 sticky 与横向滚动示例;调整列宽测量逻辑;在虚拟表格中引入 minWidth 参与宽度计算;变更 FixedHolder 对横向滚动的传参接口(由 scrollTableStyle 改为 scrollX);修正空列组渲染;版本号小幅提升。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Table as Table
participant FixedHolder as FixedHolder (Header)
participant Header as TableComponent (Header DOM)
User->>Table: 渲染含横向滚动的表格
Table->>FixedHolder: props { scrollX: mergedScrollX, ... }
FixedHolder->>Header: 应用样式 { minWidth: 100%, width: scrollX }
Header-->>User: 渲染固定表头(宽度受 scrollX 控制)
sequenceDiagram
autonumber
participant VT as VirtualTable
participant BG as BodyGrid
participant Col as Columns
VT->>BG: 计算列布局
loop 各列
BG->>Col: 读取 width, minWidth
BG-->>BG: finalWidth = max(width, minWidth)
BG-->>VT: 返回 [key, finalWidth, total]
end
VT-->>VT: 基于 finalWidth 布局/滚动
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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 @crazyair, 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 significantly improves the handling of table column widths, particularly in scenarios involving horizontal scrolling and fixed headers. The changes ensure that column widths are calculated and applied more accurately, addressing layout inconsistencies and enhancing the overall table rendering experience. New examples have been added to validate these improvements against reported issues.
Highlights
- Table Column Width Handling: Refactored the
FixedHolder
component to directly apply thescrollX
property as the table's width, ensuring more accurate and consistent column width behavior, especially in tables with fixed headers and horizontal scrolling. This change also includes settingmin-width: 100%
for the table. - Column Measurement Accuracy: Modified
MeasureCell
to prevent rendering column titles when a column has an explicit width defined. This adjustment helps in more precise column width measurement and layout calculations. - Virtualized Table Column Widths: Enhanced the column width calculation logic within
VirtualTable/BodyGrid.tsx
to correctly account for bothwidth
andminWidth
properties, improving the rendering of virtualized tables. - Conditional ColGroup Rendering: Updated
ColGroup
to conditionally render the<colgroup>
element only when there are actual columns present, preventing the rendering of emptycolgroup
tags and streamlining the table's DOM structure. - New Examples and Issue References: Added new examples in
docs/examples/scrollY.tsx
anddocs/examples/stickyHeader.tsx
to demonstrate and test specific column alignment andscroll.x
behaviors, referencing related GitHub issues (#54889 and #54894). - CSS Styling Adjustment: Removed the
word-break: break-word;
CSS property from table cells inassets/index.less
, which may affect text wrapping behavior within columns. - Version Bump: The package version has been incremented from
7.52.5
to7.52.6
inpackage.json
.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
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 issue 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. ↩
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
This pull request attempts to optimize column width measurement by avoiding rendering the title when a width is already specified. However, this change introduces a critical bug in src/Body/MeasureCell.tsx
that could lead to a runtime error. The code does not check if the column
prop is defined before using the in
operator, which will throw a TypeError
if column
is undefined
. I have provided a fix for this issue.
> | ||
<div style={{ height: 0, overflow: 'hidden', fontWeight: 'bold' }}> | ||
{column?.title || '\xa0'} | ||
{('width' in column ? '' : column?.title) || '\xa0'} |
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.
The column
prop is optional and can be undefined
. Using the in
operator on an undefined
value will cause a TypeError
at runtime. You should add a null check for column
before attempting to access its properties or use the in
operator.
{('width' in column ? '' : column?.title) || '\xa0'} | |
{(column && 'width' in column ? '' : column?.title) || '\xa0'} |
有问题重现么 |
这个改了demo,你验证那个demo是否可用 |
> | ||
<div style={{ height: 0, overflow: 'hidden', fontWeight: 'bold' }}> | ||
{column?.title || '\xa0'} | ||
{('width' in column ? '' : column?.title) || '\xa0'} |
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.
就是如果传了 width,就不渲染 title 了,还用空占位
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.
没效果,还是 width: auto 的问题,去掉就好了。之前 scroll={{ x: true }}
时默认设置的就是 width: auto
,现在可以考虑去掉了。
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.
也可能吧,我调试的时候,宽度一直不生效,还以为确实有问题,所以才改的。其实是列太多,导致 width 200 本来就会被挤掉的吧
Summary by CodeRabbit