fix(table): stretch columns correctly when movable rows are enabled#4125
Open
viktorSoftDev wants to merge 2 commits into
Open
fix(table): stretch columns correctly when movable rows are enabled#4125viktorSoftDev wants to merge 2 commits into
viktorSoftDev wants to merge 2 commits into
Conversation
The drag-handle row-header column is clamped to --limel-table-drag-handle-width by CSS, but Tabulator did not know about that width. Width-distributing layouts such as fitColumns (layout="stretchColumns") handed the handle column a full flex share, and the difference between the allocated share and the rendered CSS width showed up as a blank filler column while the data columns were left too narrow. Resolve the CSS custom property to pixels with a probe element and reserve that exact width (width + minWidth) in the rowHeader column definition, so Tabulator's layout math agrees with the rendered width. fixes #4123
Contributor
📝 WalkthroughWalkthroughThis PR fixes the issue where Tabulator's ChangesTable drag-handle width fix for stretchColumns layout
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4125/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/table/table.e2e.tsx`:
- Around line 224-233: The test currently hardcodes expectedWidth = 32 which is
brittle; instead read the CSS variable --limel-table-drag-handle-width from the
test environment (via getComputedStyle on document.documentElement), parse its
value and convert to pixels if it’s in rem (multiply by root font-size) or use
the px value directly, then use that numeric pixel value for the assertions
against handleCell.getBoundingClientRect().width and
handleHeader.getBoundingClientRect().width so the test adapts to overrides.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ed06a93-2954-466f-abc3-ce86e909c86b
📒 Files selected for processing (5)
src/components/chip-set/chip-set-input-helpers.tssrc/components/table/row-drag-manager.spec.tssrc/components/table/row-drag-manager.tssrc/components/table/table.e2e.tsxsrc/components/table/table.tsx
💤 Files with no reviewable changes (1)
- src/components/chip-set/chip-set-input-helpers.ts
Comment on lines
+224
to
+233
| // 2rem at the default 16px root font size | ||
| const expectedWidth = 32; | ||
| expect(handleCell.getBoundingClientRect().width).toBeCloseTo( | ||
| expectedWidth, | ||
| 0 | ||
| ); | ||
| expect(handleHeader.getBoundingClientRect().width).toBeCloseTo( | ||
| expectedWidth, | ||
| 0 | ||
| ); |
Contributor
There was a problem hiding this comment.
Avoid hardcoded 32px in width assertion
This assertion can become flaky when root font-size or --limel-table-drag-handle-width is overridden, even if behavior is correct. Derive expected width from the CSS variable in the test environment.
Suggested test adjustment
- // 2rem at the default 16px root font size
- const expectedWidth = 32;
+ const probe = document.createElement('div');
+ probe.style.cssText =
+ 'position:absolute; visibility:hidden;' +
+ 'width: var(--limel-table-drag-handle-width);';
+ root.shadowRoot!.append(probe);
+ const expectedWidth = probe.getBoundingClientRect().width || 32;
+ probe.remove();
expect(handleCell.getBoundingClientRect().width).toBeCloseTo(
expectedWidth,
0
);📝 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.
Suggested change
| // 2rem at the default 16px root font size | |
| const expectedWidth = 32; | |
| expect(handleCell.getBoundingClientRect().width).toBeCloseTo( | |
| expectedWidth, | |
| 0 | |
| ); | |
| expect(handleHeader.getBoundingClientRect().width).toBeCloseTo( | |
| expectedWidth, | |
| 0 | |
| ); | |
| const probe = document.createElement('div'); | |
| probe.style.cssText = | |
| 'position:absolute; visibility:hidden;' + | |
| 'width: var(--limel-table-drag-handle-width);'; | |
| root.shadowRoot!.append(probe); | |
| const expectedWidth = probe.getBoundingClientRect().width || 32; | |
| probe.remove(); | |
| expect(handleCell.getBoundingClientRect().width).toBeCloseTo( | |
| expectedWidth, | |
| 0 | |
| ); | |
| expect(handleHeader.getBoundingClientRect().width).toBeCloseTo( | |
| expectedWidth, | |
| 0 | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/table/table.e2e.tsx` around lines 224 - 233, The test
currently hardcodes expectedWidth = 32 which is brittle; instead read the CSS
variable --limel-table-drag-handle-width from the test environment (via
getComputedStyle on document.documentElement), parse its value and convert to
pixels if it’s in rem (multiply by root font-size) or use the px value directly,
then use that numeric pixel value for the assertions against
handleCell.getBoundingClientRect().width and
handleHeader.getBoundingClientRect().width so the test adapts to overrides.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Tests
Fixes #4123
What
layout="stretchColumns"did not stretch the columns whenmovableRowswas enabled — the rows ended short of the table width, leaving a blank filler column on the right.Why
The drag-handle row-header column is clamped to
--limel-table-drag-handle-width(2rem) by CSS with!important, but Tabulator's layout math knew nothing about that width. ThefitColumnslayout (whichstretchColumnsmaps to) treated the handle as a regular flexible column and handed it a full equal flex share; the difference between the allocated share and the rendered 32px showed up as blank space, and the data columns were left narrower than they should be.Note: the issue's suspected cause (
frozen: trueon the row header) was disproven — plain Tabulator 6.3.1 with a frozenrowHeaderdistributesfitColumnswidths correctly. The column stays frozen.How
The table now resolves
--limel-table-drag-handle-widthto pixels by measuring a probe element (so any unit or consumer override works) and reserves exactly that width (width+minWidth) in therowHeadercolumn definition. Tabulator's bookkeeping and the rendered width now agree, for all layouts. SettingminWidthalso fixes the header cell previously rendering at Tabulator's 40px default minimum instead of 32px.Regression-tested with two new e2e tests: rows span the full table width with
stretchColumns+movableRows, and the drag-handle column stays at its fixed width.Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: