feat(Table): update reset column width/order method#7850
Merged
Conversation
Contributor
Reviewer's GuideRefactors the table’s first-render handling and centralizes column reset logic so that column order/width/visibility are reloaded both after first render and on subsequent parameter updates, while simplifying JS table width reset calculations to rely directly on col widths. Class diagram for updated Table component first-render and column reset logicclassDiagram
class Table_TItem_Component {
- bool _firstRender
- List~ITableColumn~ Columns
- DynamicContext DynamicContext
- bool AutoGenerateColumns
- Func~IEnumerable~ITableColumn~~ ColumnOrderCallback
- Func~IEnumerable~ITableColumn~,Task~ OnColumnCreating
- string SortName
- SortOrder SortOrder
- bool ShowSkeleton
- bool IsTree
- object SearchModel
- IEnumerable~ITableColumn~ _searchItems
- bool _autoQuery
+ Table_TItem_Component()
- void OnInitParameters()
+ void OnParametersSet()
+ Task OnParametersSetAsync()
+ Task OnAfterRenderAsync(bool firstRender)
- Task OnTableRenderAsync(bool firstRender)
- Task ProcessFirstRender()
- Task OnTableColumnReset()
- void ResetDynamicContext()
- void InternalResetVisibleColumns(List~ITableColumn~ cols)
- Task ReloadColumnOrdersFromBrowserAsync(List~ITableColumn~ cols)
- Task ReloadColumnWidthFromBrowserAsync(List~ITableColumn~ cols)
- Task ReloadColumnVisibleFromBrowserAsync()
- TItem CreateTItem()
- object CreateSearchModel()
}
class ITableColumn {
<<interface>>
+ bool Sortable
+ bool DefaultSort
+ SortOrder DefaultSortOrder
+ string GetFieldName()
}
class DynamicContext {
+ IEnumerable~ITableColumn~ GetColumns()
}
class Utility {
+ static IEnumerable~ITableColumn~ GetTableColumns~TItem~(IEnumerable~ITableColumn~ columns)
}
class ColumnExtensions {
+ static IEnumerable~ITableColumn~ OrderFunc(this IEnumerable~ITableColumn~ columns)
}
class SortOrder {
<<enum>>
Ascending
Descending
None
}
Table_TItem_Component --> ITableColumn : uses
Table_TItem_Component --> DynamicContext : uses
Table_TItem_Component --> Utility : uses
Table_TItem_Component --> ColumnExtensions : uses
ITableColumn --> SortOrder : uses
Flow diagram for simplified JS table width reset and default width calculationflowchart TD
A["resetTableWidth(table)"] --> B["Iterate table.tables"]
B --> C["Find COLGROUP child of table element"]
C -->|group exists| D["Compute total width from group children"]
C -->|no group| B
D --> E["For each col in group.children<br/>parseInt(col.style.width)"]
E --> F{"Is width NaN?"}
F -->|yes| G["Use default 100"]
F -->|no| H["Use parsed width"]
G --> I["Accumulate width"]
H --> I["Accumulate width"]
I --> J["Set t.style.width to total width"]
J --> K["saveColumnWidth(table)"]
L["setTableDefaultWidth(table)"] --> M{"table.tables.length > 0<br/>and first table is visible?"}
M -->|no| N["Return"]
M -->|yes| O["Get options.scrollWidth and options.columnMinWidth"]
O --> P["Query all col elements from first table"]
P --> Q["For each col<br/>parseFloat(col.style.width)"]
Q --> R{"Is width NaN?"}
R -->|yes| S["Use columnMinWidth"]
R -->|no| T["Use parsed width"]
S --> U["Accumulate tableWidth"]
T --> U["Accumulate tableWidth"]
U --> V{"tableWidth > table.el.offsetWidth?"}
V -->|yes| W["Set table.tables[0].style.width to tableWidth"]
V -->|no| X["Do not set explicit width"]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Changing
FirstRenderfrom a protected property to a private field (_firstRender) may be a breaking change for any derived components that referenced it; consider keeping a protected read-only property or compatibility shim if inheritance is expected. - In
resetTableWidthyou useparseInt(col.style.width)whilesetTableDefaultWidthusesparseFloat; for consistency and to handle non-integer widths, consider usingparseFloatin both places. OnParametersSetAsyncnow callsOnTableColumnReseton every non-first parameter set, which could be expensive; consider guarding this with a more specific condition (e.g., only when relevant parameters change) or an explicit flag to avoid unnecessary recomputation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Changing `FirstRender` from a protected property to a private field (`_firstRender`) may be a breaking change for any derived components that referenced it; consider keeping a protected read-only property or compatibility shim if inheritance is expected.
- In `resetTableWidth` you use `parseInt(col.style.width)` while `setTableDefaultWidth` uses `parseFloat`; for consistency and to handle non-integer widths, consider using `parseFloat` in both places.
- `OnParametersSetAsync` now calls `OnTableColumnReset` on every non-first parameter set, which could be expensive; consider guarding this with a more specific condition (e.g., only when relevant parameters change) or an explicit flag to avoid unnecessary recomputation.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the Table component’s column reset behavior (width/order/visibility) to address issue #7849 by refactoring the column-reset pipeline in the Blazor component and adjusting how table widths are recalculated in the JS interop layer.
Changes:
- Refactors column reset/reload logic into a shared
OnTableColumnReset()method and invokes it on first render and subsequent parameter updates. - Updates JS logic for recalculating table widths (including default-width computation).
- Bumps package version to
10.5.1-beta06.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/BootstrapBlazor/Components/Table/Table.razor.js |
Changes table width reset/default-width calculations used by resize/reset flows. |
src/BootstrapBlazor/Components/Table/Table.razor.cs |
Refactors first-render tracking and introduces OnTableColumnReset() + OnParametersSetAsync() reload behavior. |
src/BootstrapBlazor/Components/Table/Table.razor |
Switches first-render UI gating from FirstRender to _firstRender. |
src/BootstrapBlazor/BootstrapBlazor.csproj |
Version bump. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1156
to
+1164
| protected override async Task OnParametersSetAsync() | ||
| { | ||
| await base.OnParametersSetAsync(); | ||
|
|
||
| if (!_firstRender) | ||
| { | ||
| // 重新读取浏览器设置 | ||
| await OnTableColumnReset(); | ||
| } |
There was a problem hiding this comment.
OnParametersSetAsync calls base.OnParametersSetAsync(), and ComponentBase's base implementation invokes OnParametersSet(). Because this component already overrides OnParametersSet(), this will cause OnParametersSet() to run twice per parameter update, duplicating work (and potentially triggering side effects) after the first render. Remove the base.OnParametersSetAsync() call (or move all logic into a single override) so OnParametersSet executes only once per update.
| protected bool FirstRender { get; set; } = true; | ||
| private bool _firstRender = true; | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
The protected FirstRender property was removed and replaced with a private field. Since Table is a public component, removing a protected member is a breaking change for anyone deriving from Table and using FirstRender in subclasses. Consider keeping the protected property (possibly forwarding to _firstRender and marking it [Obsolete]) to preserve backward compatibility.
Suggested change
| /// <summary> | |
| /// <summary> | |
| /// <para lang="zh">获得/设置 首次渲染标志。已弃用，请勿在派生类中继续使用。</para> | |
| /// <para lang="en">Gets or sets the first-render flag. Obsolete; do not continue using this in derived classes.</para> | |
| /// </summary> | |
| [Obsolete("已弃用，请勿继续使用 FirstRender。Deprecated, please do not continue using FirstRender.")] | |
| protected bool FirstRender | |
| { | |
| get => _firstRender; | |
| set => _firstRender = value; | |
| } | |
| /// <summary> |
Comment on lines
+1290
to
+1295
| // set default sortName | ||
| var col = Columns.Find(i => i is { Sortable: true, DefaultSort: true }); | ||
| if (col != null) | ||
| { | ||
| SortName = col.GetFieldName(); | ||
| SortOrder = col.DefaultSortOrder; |
There was a problem hiding this comment.
OnTableColumnReset resets SortName/SortOrder to the default sort column every time it runs. Since this method is now also invoked from OnParametersSetAsync on subsequent parameter updates, it can unintentionally override user-driven sorting state. Only apply the default sort when sorting is currently unset (e.g., SortName is null/empty and SortOrder is Unset) or restrict this logic to first render.
Suggested change
| // set default sortName | |
| var col = Columns.Find(i => i is { Sortable: true, DefaultSort: true }); | |
| if (col != null) | |
| { | |
| SortName = col.GetFieldName(); | |
| SortOrder = col.DefaultSortOrder; | |
| // set default sortName only when sort is not already initialized | |
| if (string.IsNullOrEmpty(SortName) && SortOrder == SortOrder.Unset) | |
| { | |
| var col = Columns.Find(i => i is { Sortable: true, DefaultSort: true }); | |
| if (col != null) | |
| { | |
| SortName = col.GetFieldName(); | |
| SortOrder = col.DefaultSortOrder; | |
| } |
Comment on lines
538
to
552
| const resetTableWidth = table => { | ||
| const { options: { scrollWidth } } = table; | ||
| table.tables.forEach(t => { | ||
| const group = [...t.children].find(i => i.nodeName === 'COLGROUP') | ||
| if (group) { | ||
| const colgroup = getLastColgroup(t, group); | ||
| if (colgroup) { | ||
| colgroup.style = null; | ||
| } | ||
| const width = getTableWidthByColumnGroup(t, 100); | ||
| if (width >= t.parentElement.offsetWidth + scrollWidth) { | ||
| t.style.width = `${width}px`; | ||
| } | ||
| else { | ||
| t.style.width = null; | ||
| } | ||
| let width = 0; | ||
| [...group.children].forEach(col => { | ||
| let colWidth = parseInt(col.style.width); | ||
| if (isNaN(colWidth)) { | ||
| colWidth = 100; | ||
| } | ||
| width += colWidth; | ||
| }) | ||
| t.style.width = `${width}px`; | ||
|
|
||
| saveColumnWidth(table); |
There was a problem hiding this comment.
resetTableWidth now always forces each table's width to the sum of colgroup widths and no longer accounts for fixed-header mode (header vs body width needing scrollWidth compensation) or the previous conditional behavior that cleared width when not needed. This can cause header/body misalignment and unnecessary horizontal scrolling. Align this logic with setTableDefaultWidth / resize-drag behavior (e.g., subtract scrollWidth for the fixed-body table and only set explicit width when required).
Comment on lines
1051
to
+1058
| if (table.tables.length > 0 && isVisible(table.tables[0])) { | ||
| const { scrollWidth, columnMinWidth } = table.options; | ||
| const tableWidth = getTableWidthByColumnGroup(table.tables[0], columnMinWidth); | ||
| const tableWidth = [...table.tables[0].querySelectorAll('col')] | ||
| .map(i => { | ||
| const colWidth = parseFloat(i.style.width); | ||
| return isNaN(colWidth) ? columnMinWidth : colWidth; | ||
| }) | ||
| .reduce((accumulator, val) => accumulator + val, 0); |
There was a problem hiding this comment.
setTableDefaultWidth now calculates tableWidth purely from col.style.width with a columnMinWidth fallback. For columns without an explicit width (style is empty), this can significantly overestimate the real rendered width and force an unnecessary fixed width + horizontal scroll. The previous implementation measured actual header/body cell widths when style width was absent. Consider restoring that measurement-based fallback (offsetWidth) rather than defaulting to columnMinWidth for unset widths.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7850 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 764 764
Lines 34165 34175 +10
Branches 4704 4705 +1
=========================================
+ Hits 34165 34175 +10
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:
|
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
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.
Link issues
fixes #7849
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Update table column reset behavior to reapply persisted column order, width, and visibility on parameter changes and adjust default width calculation in the JavaScript table utilities.
Enhancements: