Conversation
# Conflicts: # src/BootstrapBlazor/BootstrapBlazor.csproj
Reviewer's GuideRefactors table width recalculation to derive widths from rendered column groups and cells, adds helpers to handle fixed-header colgroups and column width fallback logic, and slightly re-indents a Checkbox render fragment in the Razor markup. Flow diagram for getColumnRenderWidth fallback logicflowchart TD
A[getColumnRenderWidth table, col, index, defaultWidth] --> B[Parse width from col.style.width]
B --> C{width is a valid positive number?}
C -- Yes --> D[Return width]
C -- No --> E[Get header = thead > tr:last-child > th at index]
E --> F[width = header.offsetWidth or 0]
F --> G{width > 0?}
G -- Yes --> H[Return width]
G -- No --> I[Find first tbody tr not having class is-detail]
I --> J[Get cell = row.children.item index]
J --> K[width = cell.offsetWidth or 0]
K --> L{width > 0?}
L -- Yes --> M[Return width]
L -- No --> N[Return defaultWidth]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
getLastColgroup, thelength - 2/length - 1indexing assumes a minimum number ofcolelements; consider guarding againstlength < 2to avoid returningundefinedor accessing out-of-bounds when table markup changes. getColumnRenderWidthrepeatedly queriestheadandtbodyfor each column; if this runs frequently, you may want to cache the header row and a representative body row outside the loop and pass them in to avoid repeated DOM queries.- In
resetTableWidth,colgroup.style = nullwipes all inline styles; if there are non-width-related styles that need to be preserved, consider clearing onlycolgroup.style.widthinstead of resetting the whole style object.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `getLastColgroup`, the `length - 2`/`length - 1` indexing assumes a minimum number of `col` elements; consider guarding against `length < 2` to avoid returning `undefined` or accessing out-of-bounds when table markup changes.
- `getColumnRenderWidth` repeatedly queries `thead` and `tbody` for each column; if this runs frequently, you may want to cache the header row and a representative body row outside the loop and pass them in to avoid repeated DOM queries.
- In `resetTableWidth`, `colgroup.style = null` wipes all inline styles; if there are non-width-related styles that need to be preserved, consider clearing only `colgroup.style.width` instead of resetting the whole style object.
## Individual Comments
### Comment 1
<location path="src/BootstrapBlazor/Components/Table/Table.razor.js" line_range="1067-1088" />
<code_context>
}));
}
+const getTableWidthByColumnGroup = (table, defaultWidth) => {
+ return [...table.querySelectorAll('colgroup col')]
+ .map((col, index) => getColumnRenderWidth(table, col, index, defaultWidth))
+ .reduce((accumulator, val) => accumulator + val, 0);
+}
+
+const getColumnRenderWidth = (table, col, index, defaultWidth) => {
+ let width = parseFloat(col.style.width);
+ if (!isNaN(width) && width > 0) {
+ return width;
+ }
+
+ const header = table.querySelectorAll('thead > tr:last-child > th').item(index);
+ width = header?.offsetWidth ?? 0;
+ if (width > 0) {
+ return width;
+ }
+
+ const row = [...table.querySelectorAll('tbody > tr')].find(i => !i.classList.contains('is-detail'));
+ width = row?.children.item(index)?.offsetWidth ?? 0;
+ return width > 0 ? width : defaultWidth;
+}
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid repeated DOM queries inside width calculations for better performance.
`getTableWidthByColumnGroup` / `getColumnRenderWidth` re-run `querySelectorAll` on `colgroup col`, `thead`, and `tbody` for every column, which can be costly on large tables or during frequent resizes. Consider querying these once (e.g. `const cols = ...`, `const headers = ...`, a cached non-detail body row) and passing them into `getColumnRenderWidth`, or at least moving the header/body queries outside the per-column loop.
```suggestion
const getTableWidthByColumnGroup = (table, defaultWidth) => {
const cols = [...table.querySelectorAll('colgroup col')];
const headers = table.querySelectorAll('thead > tr:last-child > th');
const bodyRows = table.querySelectorAll('tbody > tr');
const dataRow = [...bodyRows].find(row => !row.classList.contains('is-detail'));
return cols
.map((col, index) => getColumnRenderWidth(col, index, defaultWidth, headers, dataRow))
.reduce((accumulator, val) => accumulator + val, 0);
}
const getColumnRenderWidth = (col, index, defaultWidth, headers, dataRow) => {
let width = parseFloat(col.style.width);
if (!isNaN(width) && width > 0) {
return width;
}
const header = headers.item(index);
width = header?.offsetWidth ?? 0;
if (width > 0) {
return width;
}
const cell = dataRow?.children.item(index);
width = cell?.offsetWidth ?? 0;
return width > 0 ? width : defaultWidth;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const getTableWidthByColumnGroup = (table, defaultWidth) => { | ||
| return [...table.querySelectorAll('colgroup col')] | ||
| .map((col, index) => getColumnRenderWidth(table, col, index, defaultWidth)) | ||
| .reduce((accumulator, val) => accumulator + val, 0); | ||
| } | ||
|
|
||
| const getColumnRenderWidth = (table, col, index, defaultWidth) => { | ||
| let width = parseFloat(col.style.width); | ||
| if (!isNaN(width) && width > 0) { | ||
| return width; | ||
| } | ||
|
|
||
| const header = table.querySelectorAll('thead > tr:last-child > th').item(index); | ||
| width = header?.offsetWidth ?? 0; | ||
| if (width > 0) { | ||
| return width; | ||
| } | ||
|
|
||
| const row = [...table.querySelectorAll('tbody > tr')].find(i => !i.classList.contains('is-detail')); | ||
| width = row?.children.item(index)?.offsetWidth ?? 0; | ||
| return width > 0 ? width : defaultWidth; | ||
| } |
There was a problem hiding this comment.
suggestion (performance): Avoid repeated DOM queries inside width calculations for better performance.
getTableWidthByColumnGroup / getColumnRenderWidth re-run querySelectorAll on colgroup col, thead, and tbody for every column, which can be costly on large tables or during frequent resizes. Consider querying these once (e.g. const cols = ..., const headers = ..., a cached non-detail body row) and passing them into getColumnRenderWidth, or at least moving the header/body queries outside the per-column loop.
| const getTableWidthByColumnGroup = (table, defaultWidth) => { | |
| return [...table.querySelectorAll('colgroup col')] | |
| .map((col, index) => getColumnRenderWidth(table, col, index, defaultWidth)) | |
| .reduce((accumulator, val) => accumulator + val, 0); | |
| } | |
| const getColumnRenderWidth = (table, col, index, defaultWidth) => { | |
| let width = parseFloat(col.style.width); | |
| if (!isNaN(width) && width > 0) { | |
| return width; | |
| } | |
| const header = table.querySelectorAll('thead > tr:last-child > th').item(index); | |
| width = header?.offsetWidth ?? 0; | |
| if (width > 0) { | |
| return width; | |
| } | |
| const row = [...table.querySelectorAll('tbody > tr')].find(i => !i.classList.contains('is-detail')); | |
| width = row?.children.item(index)?.offsetWidth ?? 0; | |
| return width > 0 ? width : defaultWidth; | |
| } | |
| const getTableWidthByColumnGroup = (table, defaultWidth) => { | |
| const cols = [...table.querySelectorAll('colgroup col')]; | |
| const headers = table.querySelectorAll('thead > tr:last-child > th'); | |
| const bodyRows = table.querySelectorAll('tbody > tr'); | |
| const dataRow = [...bodyRows].find(row => !row.classList.contains('is-detail')); | |
| return cols | |
| .map((col, index) => getColumnRenderWidth(col, index, defaultWidth, headers, dataRow)) | |
| .reduce((accumulator, val) => accumulator + val, 0); | |
| } | |
| const getColumnRenderWidth = (col, index, defaultWidth, headers, dataRow) => { | |
| let width = parseFloat(col.style.width); | |
| if (!isNaN(width) && width > 0) { | |
| return width; | |
| } | |
| const header = headers.item(index); | |
| width = header?.offsetWidth ?? 0; | |
| if (width > 0) { | |
| return width; | |
| } | |
| const cell = dataRow?.children.item(index); | |
| width = cell?.offsetWidth ?? 0; | |
| return width > 0 ? width : defaultWidth; | |
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7836 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 764 764
Lines 34147 34147
Branches 4701 4701
=========================================
Hits 34147 34147
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the Table component’s client-side width reset logic to address incorrect table width after resizing columns and then toggling column visibility (issue #7835), aiming to ensure the table properly fills its parent container.
Changes:
- Reworked
resetTableWidthto conditionally clear/set the table width based on computed column render widths and available container width. - Added helpers to compute table width using colgroup + rendered header/body cell widths as fallbacks.
- Bumped
BootstrapBlazorpackage version to10.5.1-beta02(plus a small Razor formatting adjustment).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/BootstrapBlazor/Components/Table/Table.razor.js |
Updates/reset logic for table width and adds width-calculation helpers. |
src/BootstrapBlazor/Components/Table/Table.razor |
Minor formatting change in the column visibility dropdown item. |
src/BootstrapBlazor/BootstrapBlazor.csproj |
Version bump to 10.5.1-beta02. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| t.style.width = `${width}px`; | ||
| const colgroup = getLastColgroup(t, group); | ||
| if (colgroup) { | ||
| colgroup.style = null; |
There was a problem hiding this comment.
colgroup.style = null attempts to assign to the DOM style property (a read-only CSSStyleDeclaration in ES modules/strict mode) and can throw a TypeError. If the intent is to clear the last column's inline width, use colgroup.style.removeProperty('width'), set colgroup.style.width = '', or colgroup.removeAttribute('style') (depending on what needs to be cleared).
| colgroup.style = null; | |
| colgroup.style.width = ''; |
| t.style.width = `${width}px`; | ||
| } | ||
| else { | ||
| t.style.width = null; |
There was a problem hiding this comment.
Using t.style.width = null is unreliable because the setter coerces null to a string and may not remove the style (potentially leaving a stale width). Use t.style.removeProperty('width') (or set t.style.width = '') to explicitly clear the inline width when the table should auto-size to its container.
| t.style.width = null; | |
| t.style.removeProperty('width'); |
| const getLastColgroup = (table, group) => { | ||
| const p = table.parentElement; | ||
| if (p) { | ||
| const length = group.children.length; | ||
| if (p.classList.contains("table-fixed-header")) { | ||
| return group.children[length - 2]; | ||
| } | ||
| else { | ||
| return group.children[length - 1]; | ||
| } |
There was a problem hiding this comment.
getLastColgroup returns a col element (a child of the COLGROUP), not a COLGROUP. The current name/variable (colgroup) is misleading and makes the logic harder to follow. Consider renaming to something like getLastColElement / getLastDataCol and renaming the local variable accordingly.
| return [...table.querySelectorAll('colgroup col')] | ||
| .map((col, index) => getColumnRenderWidth(table, col, index, defaultWidth)) | ||
| .reduce((accumulator, val) => accumulator + val, 0); | ||
| } | ||
|
|
||
| const getColumnRenderWidth = (table, col, index, defaultWidth) => { | ||
| let width = parseFloat(col.style.width); | ||
| if (!isNaN(width) && width > 0) { | ||
| return width; | ||
| } | ||
|
|
||
| const header = table.querySelectorAll('thead > tr:last-child > th').item(index); | ||
| width = header?.offsetWidth ?? 0; | ||
| if (width > 0) { | ||
| return width; | ||
| } | ||
|
|
||
| const row = [...table.querySelectorAll('tbody > tr')].find(i => !i.classList.contains('is-detail')); |
There was a problem hiding this comment.
getTableWidthByColumnGroup/getColumnRenderWidth calls querySelectorAll for the header and searches tbody rows once per column, which scales poorly with many columns (O(n^2) DOM queries). Consider caching the header NodeList and the first non-detail row once per table-width calculation and passing them into getColumnRenderWidth to reduce repeated DOM work.
| return [...table.querySelectorAll('colgroup col')] | |
| .map((col, index) => getColumnRenderWidth(table, col, index, defaultWidth)) | |
| .reduce((accumulator, val) => accumulator + val, 0); | |
| } | |
| const getColumnRenderWidth = (table, col, index, defaultWidth) => { | |
| let width = parseFloat(col.style.width); | |
| if (!isNaN(width) && width > 0) { | |
| return width; | |
| } | |
| const header = table.querySelectorAll('thead > tr:last-child > th').item(index); | |
| width = header?.offsetWidth ?? 0; | |
| if (width > 0) { | |
| return width; | |
| } | |
| const row = [...table.querySelectorAll('tbody > tr')].find(i => !i.classList.contains('is-detail')); | |
| const headers = table.querySelectorAll('thead > tr:last-child > th'); | |
| const rows = table.querySelectorAll('tbody > tr'); | |
| const row = [...rows].find(i => !i.classList.contains('is-detail')); | |
| return [...table.querySelectorAll('colgroup col')] | |
| .map((col, index) => getColumnRenderWidth(col, index, defaultWidth, headers, row)) | |
| .reduce((accumulator, val) => accumulator + val, 0); | |
| } | |
| const getColumnRenderWidth = (col, index, defaultWidth, headers, row) => { | |
| let width = parseFloat(col.style.width); | |
| if (!isNaN(width) && width > 0) { | |
| return width; | |
| } | |
| const header = headers.item(index); | |
| width = header?.offsetWidth ?? 0; | |
| if (width > 0) { | |
| return width; | |
| } |
Link issues
fixes #7835
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Adjust table width recalculation to use rendered column widths and support fixed-header layouts when resetting and initializing table widths.
Bug Fixes:
Enhancements: