refactor(Table): redesign auto fill column width logic#7956
Merged
Conversation
Contributor
Reviewer's GuideRefactors the table column width persistence and auto-fit logic to use a unified column state model (name, width, visibility, table width) stored in localStorage, updates JS–.NET interop to pass the full state instead of individual widths, and replaces the old ColumnVisible/ColumnWidth types and APIs with a consolidated TableColumnState-based implementation. Sequence diagram for column resize and state persistencesequenceDiagram
participant U as User
participant B as Browser_Table_JS
participant LS as localStorage
participant C as Blazor_Table_Component
U->>B: Drag column resizer / double click
B->>B: autoFitColumnWidth(table,col)
B->>B: getColumnStateObject(table)
B->>LS: setItem(bb-table-column-tableName, stateJson)
B->>C: invokeMethodAsync(ResizeColumnCallback, field, stateJson)
C->>C: ResizeColumnCallback(name, JsonElement state)
C->>C: state.Parse<TableColumnLocalstorageStatus>()
C->>C: Update _tableColumnStateCache.Columns widths
C->>C: Update _tableColumnStateCache.TableWidth
alt OnResizeColumnAsync set
C-->>C: OnResizeColumnAsync(name, column.Width)
end
C-->>B: return (JS interop complete)
B->>B: resetColumnWidthTips(table,col)
Class diagram for unified Table column state modelclassDiagram
class TableColumnState {
+string Name
+string? DisplayName
+bool Visible
+int? Width
}
class TableColumnLocalstorageStatus {
+List~TableColumnState~ Columns
+int TableWidth
}
class Table~TItem~ {
-TableColumnLocalstorageStatus _tableColumnStateCache
-List~TableColumnState~ _columnVisibleItems
+Task ReloadColumnStatesFromBrowserAsync()
-void RebuildTableColumnFromCache(List~ITableColumn~ cols)
-List~TableColumnState~ GetColumnVisibleItems(List~ITableColumn~ cols)
+void ResetVisibleColumns(IEnumerable~TableColumnState~ columns)
+Task ResizeColumnCallback(string name, JsonElement state)
-Task OnToggleColumnVisible(TableColumnState item, bool visible)
}
class JsonElementExtensions {
+T? Parse~T~(JsonSerializerDefaults options)
}
class ITableColumn {
<<interface>>
+string GetFieldName()
+bool GetIgnore()
+bool GetVisible()
+string GetDisplayName()
+int? Width
+bool Visible
}
TableColumnLocalstorageStatus "1" o-- "*" TableColumnState : contains
Table~TItem~ "1" o-- "1" TableColumnLocalstorageStatus : uses_cache
Table~TItem~ "1" o-- "*" TableColumnState : visible_items
Table~TItem~ ..> JsonElementExtensions : uses_Parse
Table~TItem~ --> ITableColumn : rebuilds_from
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 found 2 issues, and left some high level feedback:
- The new JsonElementExtensions class appears to use invalid C# syntax (
extension(JsonElement element)and an innerParsemethod); this should likely be refactored into a standard static extension method signature such aspublic static T? Parse<T>(this JsonElement element, JsonSerializerDefaults options = JsonSerializerDefaults.Web)at the class level. - In
getTableWidth,colgroupis assumed to exist andcolgroup.childrenis accessed unguarded; consider handling the case where the table has no COLGROUP to avoid a runtime error in those scenarios. - The new combined column state (width + visibility) handling introduces several storage keys (
bb-table-column-*), but the old keys for width/visibility are only partially cleaned up (commented out removals ingetColumnStates); consider either fully migrating/cleaning legacy keys or documenting why they must be preserved to avoid inconsistent state between versions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new JsonElementExtensions class appears to use invalid C# syntax (`extension(JsonElement element)` and an inner `Parse` method); this should likely be refactored into a standard static extension method signature such as `public static T? Parse<T>(this JsonElement element, JsonSerializerDefaults options = JsonSerializerDefaults.Web)` at the class level.
- In `getTableWidth`, `colgroup` is assumed to exist and `colgroup.children` is accessed unguarded; consider handling the case where the table has no COLGROUP to avoid a runtime error in those scenarios.
- The new combined column state (width + visibility) handling introduces several storage keys (`bb-table-column-*`), but the old keys for width/visibility are only partially cleaned up (commented out removals in `getColumnStates`); consider either fully migrating/cleaning legacy keys or documenting why they must be preserved to avoid inconsistent state between versions.
## Individual Comments
### Comment 1
<location path="src/BootstrapBlazor/Extensions/JsonElementExtesions.cs" line_range="10-12" />
<code_context>
+
+namespace BootstrapBlazor.Components;
+
+static class JsonElementExtensions
+{
+ extension(JsonElement element)
+ {
+ public T? Parse<T>(JsonSerializerDefaults options = JsonSerializerDefaults.Web)
</code_context>
<issue_to_address>
**issue (bug_risk):** The JsonElementExtensions type is not valid C# and will not compile; it should be a static class with a proper extension method signature.
To define `Parse<T>` as an extension method on `JsonElement`, rewrite this as a static class with a proper extension signature, for example:
```csharp
namespace BootstrapBlazor.Components;
internal static class JsonElementExtensions
{
public static T? Parse<T>(this JsonElement element, JsonSerializerDefaults options = JsonSerializerDefaults.Web)
{
try
{
return element.Deserialize<T>(new JsonSerializerOptions(options));
}
catch
{
return default;
}
}
}
```
You may also want to decide whether this class should be `internal` or `public` based on where it needs to be used.
</issue_to_address>
### Comment 2
<location path="src/BootstrapBlazor/Components/Table/Table.razor.js" line_range="852" />
<code_context>
}
export function getColumnStates(tableName) {
- const columnVisibleStates = getColumnVisibleState(tableName);
+ const state = getColumnStateFromLocalstorage(tableName);
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the legacy-to-new state migration and the state-building logic into dedicated helpers so `getColumnStates` and `saveColumnStateToLocalstorage` have simpler, clearer responsibilities.
You can keep the new combined column state while reducing the cognitive load by separating concerns a bit more. Two concrete areas:
---
### 1. Extract legacy–>new migration from `getColumnStates`
`getColumnStates` currently mixes:
- read new state
- read legacy width state
- merge legacy visibility into width
- (commented-out) cleanup
You can move the migration logic into a dedicated helper so `getColumnStates` is a simple “get or migrate” call:
```js
export function getColumnStates(tableName) {
return getColumnStateFromLocalstorage(tableName)
?? migrateLegacyColumnState(tableName);
}
const migrateLegacyColumnState = tableName => {
const columnWidthState = getColumnWidthState(tableName);
if (!columnWidthState) {
return null;
}
const columnVisibleStates = getColumnVisibleState(tableName);
if (columnVisibleStates) {
for (const item of columnWidthState.cols) {
const { name } = item;
const column = columnVisibleStates.find(i => i.name === name);
item.visible = column ? column.visible : true;
}
// optional: once you’re ready to remove legacy keys:
// removeColumnVisibleState(tableName);
// removeColumnWidthState(tableName);
}
return columnWidthState;
};
```
This keeps all existing behavior, but limits `getColumnStates` to a trivial control flow and localizes the migration (including the commented-out cleanup) into one place.
---
### 2. Separate building state vs persisting state
`saveColumnStateToLocalstorage` both computes the state and writes it, which makes call sites harder to reason about (sometimes it uses the passed `state`, sometimes it recomputes).
You can keep the same functionality but make the API more explicit:
```js
const buildColumnState = table => {
const cols = table.options.visibleColumns;
return {
cols: cols.map(col => ({
name: col.name,
width: getColumnWidth(col, table.columns),
visible: col.visible
})),
table: getTableWidth(table.tables[0])
};
};
const saveColumnStateToLocalstorage = (table, state) => {
const { options: { tableName } } = table;
if (!tableName) return;
const columnStateKey = `bb-table-column-${tableName}`;
const columnState = state ?? buildColumnState(table);
localStorage.setItem(columnStateKey, JSON.stringify(columnState));
};
```
Call sites that already have the state stay clear:
```js
const state = buildColumnState(table);
saveColumnStateToLocalstorage(table, state);
await table.invoke.invokeMethodAsync(table.options.resizeColumnCallback, field, state);
```
And places that don’t care about the state can keep the convenience:
```js
// e.g. in resetColumns
table.options.visibleColumns = visibleColumns;
saveColumnStateToLocalstorage(table); // computes via buildColumnState
```
This keeps all behavior intact while making “what this function does” easier to understand at each call site.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+10
to
+12
| static class JsonElementExtensions | ||
| { | ||
| extension(JsonElement element) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The JsonElementExtensions type is not valid C# and will not compile; it should be a static class with a proper extension method signature.
To define Parse<T> as an extension method on JsonElement, rewrite this as a static class with a proper extension signature, for example:
namespace BootstrapBlazor.Components;
internal static class JsonElementExtensions
{
public static T? Parse<T>(this JsonElement element, JsonSerializerDefaults options = JsonSerializerDefaults.Web)
{
try
{
return element.Deserialize<T>(new JsonSerializerOptions(options));
}
catch
{
return default;
}
}
}You may also want to decide whether this class should be internal or public based on where it needs to be used.
| @@ -880,25 +850,59 @@ | |||
| } | |||
|
|
|||
| export function getColumnStates(tableName) { | |||
Contributor
There was a problem hiding this comment.
issue (complexity): Consider extracting the legacy-to-new state migration and the state-building logic into dedicated helpers so getColumnStates and saveColumnStateToLocalstorage have simpler, clearer responsibilities.
You can keep the new combined column state while reducing the cognitive load by separating concerns a bit more. Two concrete areas:
1. Extract legacy–>new migration from getColumnStates
getColumnStates currently mixes:
- read new state
- read legacy width state
- merge legacy visibility into width
- (commented-out) cleanup
You can move the migration logic into a dedicated helper so getColumnStates is a simple “get or migrate” call:
export function getColumnStates(tableName) {
return getColumnStateFromLocalstorage(tableName)
?? migrateLegacyColumnState(tableName);
}
const migrateLegacyColumnState = tableName => {
const columnWidthState = getColumnWidthState(tableName);
if (!columnWidthState) {
return null;
}
const columnVisibleStates = getColumnVisibleState(tableName);
if (columnVisibleStates) {
for (const item of columnWidthState.cols) {
const { name } = item;
const column = columnVisibleStates.find(i => i.name === name);
item.visible = column ? column.visible : true;
}
// optional: once you’re ready to remove legacy keys:
// removeColumnVisibleState(tableName);
// removeColumnWidthState(tableName);
}
return columnWidthState;
};This keeps all existing behavior, but limits getColumnStates to a trivial control flow and localizes the migration (including the commented-out cleanup) into one place.
2. Separate building state vs persisting state
saveColumnStateToLocalstorage both computes the state and writes it, which makes call sites harder to reason about (sometimes it uses the passed state, sometimes it recomputes).
You can keep the same functionality but make the API more explicit:
const buildColumnState = table => {
const cols = table.options.visibleColumns;
return {
cols: cols.map(col => ({
name: col.name,
width: getColumnWidth(col, table.columns),
visible: col.visible
})),
table: getTableWidth(table.tables[0])
};
};
const saveColumnStateToLocalstorage = (table, state) => {
const { options: { tableName } } = table;
if (!tableName) return;
const columnStateKey = `bb-table-column-${tableName}`;
const columnState = state ?? buildColumnState(table);
localStorage.setItem(columnStateKey, JSON.stringify(columnState));
};Call sites that already have the state stay clear:
const state = buildColumnState(table);
saveColumnStateToLocalstorage(table, state);
await table.invoke.invokeMethodAsync(table.options.resizeColumnCallback, field, state);And places that don’t care about the state can keep the convenience:
// e.g. in resetColumns
table.options.visibleColumns = visibleColumns;
saveColumnStateToLocalstorage(table); // computes via buildColumnStateThis keeps all behavior intact while making “what this function does” easier to understand at each call site.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the Table component’s client-side column persistence by consolidating column visibility and width into a unified “column state” model, and revises the JS↔.NET resize callback to pass the full state payload (supporting the redesigned auto-fill/width logic from #7955).
Changes:
- Replaced the old
ColumnVisibleItem+ separate column-width state model withTableColumnState(visibility + width) and a simplified localStorage payload{ cols, table }. - Updated Table JS to save/restore the combined state and updated the JSInvokable resize callback to accept a JSON state object.
- Added a
JsonElementparsing helper extension and updated demo usage to the new state type.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/BootstrapBlazor/Extensions/JsonElementExtesions.cs | Adds a JsonElement extension to safely deserialize JS-provided JSON payloads. |
| src/BootstrapBlazor/Components/Table/TableColumnState.cs | Introduces TableColumnState (visibility + width) replacing the prior visibility-only type. |
| src/BootstrapBlazor/Components/Table/TableColumnLocalstorageStatus.cs | Simplifies persisted table state to cols + table width. |
| src/BootstrapBlazor/Components/Table/Table.razor.Toolbar.cs | Switches internal column visibility list to TableColumnState. |
| src/BootstrapBlazor/Components/Table/Table.razor.js | Reworks localStorage read/write and resize persistence to use combined column state. |
| src/BootstrapBlazor/Components/Table/Table.razor.cs | Updates state reload/apply logic and changes resize callback to accept JSON state. |
| src/BootstrapBlazor/Components/Table/Table.razor.Checkbox.cs | Updates column visibility toggle logic to work with the unified state/cache model. |
| src/BootstrapBlazor/Components/Table/Table.razor | Updates column list rendering to use TableColumnState. |
| src/BootstrapBlazor/Components/Table/ColumnWidthState.cs | Removes the old standalone persisted table-width/column-width model. |
| src/BootstrapBlazor/Components/Table/ColumnWidth.cs | Removes the old per-column width DTO used by the legacy persistence model. |
| src/BootstrapBlazor.Server/Components/Samples/Table/TablesColumnList.razor.cs | Updates sample code to call ResetVisibleColumns with TableColumnState. |
Comments suppressed due to low confidence (2)
src/BootstrapBlazor/Components/Table/TableColumnState.cs:16
- This renames/remodels the previously public
ColumnVisibleItemintoTableColumnStatewithout providing a compatibility type/alias/constructors. This is a breaking public API change for downstream apps (and existing tests in this repo) that still referenceColumnVisibleItemand its constructors (e.g.,new("Name", true)). Consider keepingColumnVisibleItemas an[Obsolete]wrapper aroundTableColumnState(or type-forwarding) and/or adding equivalent constructors onTableColumnStateto preserve source compatibility.
src/BootstrapBlazor/Components/Table/TableColumnState.cs:13 - The XML doc summary still describes this as a “column visibility” type, but the class now also persists column width (and is used as the unified client-side column state). Please update the summary to reflect its broader responsibility (visibility + width/state) to avoid misleading API docs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
90
to
98
| if (isVisible(table.el) === false) { | ||
| table.loopCheckHeightHandler = requestAnimationFrame(() => check(table)); | ||
| return; | ||
| } | ||
|
|
||
| observeHeight(table) | ||
| } | ||
|
|
||
| export function fitAllColumnWidth(id) { | ||
| const table = Data.get(id) | ||
| if (table === null) { | ||
| return; | ||
| } | ||
|
|
||
| const columns = [...table.tables[0].querySelectorAll('.col-resizer')]; | ||
| columns.forEach(async col => { | ||
| await autoFitColumnWidth(table, col); | ||
| }); | ||
| saveColumnWidth(); | ||
| } | ||
|
|
||
| const observeHeight = table => { |
Comment on lines
+865
to
+874
| for (const item of columnWidthState.cols) { | ||
| const { name } = item; | ||
| const column = columnVisibleStates.find(i => i.name === name); | ||
| if (column) { | ||
| item.visible = column.visible; | ||
| } | ||
| else { | ||
| item.visible = true; | ||
| } | ||
| } |
Comment on lines
1449
to
1456
| /// <para lang="zh">设置 列可见方法</para> | ||
| /// <para lang="en">Set Column Visible Method</para> | ||
| /// </summary> | ||
| /// <param name="columns"></param> | ||
| public void ResetVisibleColumns(IEnumerable<ColumnVisibleItem> columns) | ||
| public void ResetVisibleColumns(IEnumerable<TableColumnState> columns) | ||
| { | ||
| // https://github.com/dotnetcore/BootstrapBlazor/issues/6823 | ||
| foreach (var col in columns) |
Comment on lines
1821
to
1827
| /// <summary> | ||
| /// <para lang="zh">获得/设置 设置列宽回调方法</para> | ||
| /// <para lang="en">Gets or sets Resize Column Callback</para> | ||
| /// </summary> | ||
| [Parameter] | ||
| public Func<string, float, Task>? OnResizeColumnAsync { get; set; } | ||
| public Func<string, int?, Task>? OnResizeColumnAsync { get; set; } | ||
|
|
Comment on lines
1896
to
1904
| { | ||
| // 调整列宽时返回所有列状态，更新缓存数据中列宽度与可见性 | ||
| var columnState = state.Parse<TableColumnLocalstorageStatus>(); | ||
| if (columnState == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (!string.IsNullOrEmpty(ClientTableName)) |
Comment on lines
1296
to
+1311
| var state = await InvokeAsync<TableColumnLocalstorageStatus>("getColumnStates", ClientTableName); | ||
| if (state != null) | ||
| if (state == null) | ||
| { | ||
| state.ColumnWidthState ??= new(); | ||
| state.ColumnVisibleStates ??= new(); | ||
| state = new(); | ||
|
|
||
| _tableColumnStateCache = state; | ||
| // 开启客户端持久化后未设置列状态的列默认使用组件参数值 | ||
| state.Columns.AddRange(Columns.Where(i => !i.GetIgnore()).Select(i => new TableColumnState() | ||
| { | ||
| Name = i.GetFieldName(), | ||
| Visible = i.GetVisible(), | ||
| DisplayName = i.GetDisplayName(), | ||
| Width = i.Width | ||
| })); | ||
| } | ||
|
|
||
| _tableColumnStateCache = state; |
Comment on lines
+8
to
+11
| namespace BootstrapBlazor.Components; | ||
|
|
||
| static class JsonElementExtensions | ||
| { |
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 #7955
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Unify client-side table column width and visibility persistence into a single state model and adjust related resize, auto-fit, and rendering logic to compute and store integer widths and table width consistently.
Bug Fixes:
Enhancements: