fix(Table): incorrect grid template column on uncollapse #13496
Conversation
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
There was a problem hiding this comment.
Hey @axel7083 - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `packages/ui/src/lib/table/Table.svelte:143` </location>
<code_context>
-});
-
-function setGridColumns(): void {
+let gridTemplateColumns: string;
+$: {
// section and checkbox columns
</code_context>
<issue_to_address>
gridTemplateColumns is not initialized and may be undefined on first render.
Initialize gridTemplateColumns to an empty string or a default value to prevent potential layout issues from it being undefined.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| }); | ||
| function setGridColumns(): void { | ||
| let gridTemplateColumns: string; |
There was a problem hiding this comment.
issue (bug_risk): gridTemplateColumns is not initialized and may be undefined on first render.
Initialize gridTemplateColumns to an empty string or a default value to prevent potential layout issues from it being undefined.
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
|
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. 📝 WalkthroughWalkthroughThis change refactors the way CSS grid column widths are managed in the table component. The imperative approach using DOM queries and direct style manipulation in the Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (9)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
What does this PR do?
The current mechanism of computing the
grid-template-columnhas some flaw: every time the DOM changes, it need to be re-applied.An alternative (this PR) is to instead have a CSS property (named
table-grid-table-columns), and update thegrid-ablecss class to use it. Then we can easily set a css variable value using svelte, without having to update manually the DOM or usegetElementsByClassNamefunctions.Screenshot / video of UI
table-grid-template-css.mp4
What issues does this PR fix or reference?
Fixes #13495
How to test this PR?