-
-
Notifications
You must be signed in to change notification settings - Fork 362
feat(ISortableList): add ISortableList interface #6527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideThis PR enhances the Blazor rendering of dynamic elements and table rows by introducing a customizable Key parameter and row-key callback, enabling fine-grained control over component identity during rendering. Sequence diagram for rendering a table row with custom keysequenceDiagram
participant Table as Table<TItem>
participant DynamicElement
participant User as actor
User->>Table: Triggers table render
Table->>Table: GetKeyByITem(item)
Table->>DynamicElement: Passes Key=GetKeyByITem(item)
DynamicElement->>DynamicElement: Sets component key
DynamicElement-->>Table: Rendered with custom key
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
- Please update the PR title and description to accurately reflect that this change introduces customizable row keys for tables rather than disabling TaskService.
- To preserve existing behavior, consider defaulting the row key to the item instance when OnGetRowKey is not provided so rows remain keyed by default.
- Rename the GetKeyByITem method to something like GetRowKey or correct its casing for better clarity and consistency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Please update the PR title and description to accurately reflect that this change introduces customizable row keys for tables rather than disabling TaskService.
- To preserve existing behavior, consider defaulting the row key to the item instance when OnGetRowKey is not provided so rows remain keyed by default.
- Rename the GetKeyByITem method to something like GetRowKey or correct its casing for better clarity and consistency.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/Table/Table.razor:246` </location>
<code_context>
else
{
- <DynamicElement class="@GetRowClassString(item, "table-row")" @key="item"
+ <DynamicElement class="@GetRowClassString(item, "table-row")" Key="GetKeyByITem(item)"
TriggerContextMenu="ContextMenuZone != null" OnContextMenu="e => OnContextMenu(e, item)"
@ontouchstart="e => OnTouchStart(e, item)"
</code_context>
<issue_to_address>
The method name 'GetKeyByITem' appears to have a typo.
Please rename the method to 'GetKeyByItem' for clarity and consistency.
Suggested implementation:
```
<DynamicElement class="@GetRowClassString(item, "table-row")" Key="GetKeyByItem(item)"
```
```
@<DynamicElement TagName="tr" class="@GetRowClassString(item)" Key="GetKeyByItem(item)"
```
If there is a method definition named `GetKeyByITem` elsewhere in the file or codebase, it should also be renamed to `GetKeyByItem` to maintain consistency and avoid runtime errors.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Components/Table/Table.razor:690` </location>
<code_context>
RenderFragment<TItem> RenderRow => item =>
- @<DynamicElement TagName="tr" class="@GetRowClassString(item)" @key="item"
+ @<DynamicElement TagName="tr" class="@GetRowClassString(item)" Key="GetKeyByITem(item)"
TriggerContextMenu="ContextMenuZone != null" OnContextMenu="e => OnContextMenu(e, item)"
@ontouchstart="e => OnTouchStart(e, item)" @ontouchend="OnTouchEnd"
</code_context>
<issue_to_address>
Typo in method name 'GetKeyByITem' used for the Key parameter.
Please correct the method name to 'GetKeyByItem' to avoid confusion and potential errors.
Suggested implementation:
```
<DynamicElement class="@GetRowClassString(item, "table-row")" Key="GetKeyByItem(item)"
```
```
@<DynamicElement TagName="tr" class="@GetRowClassString(item)" Key="GetKeyByItem(item)"
```
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Components/Table/Table.razor.cs:1690` </location>
<code_context>
TouchStart = false;
}
+ private object? GetKeyByITem(TItem item) => OnGetRowKey?.Invoke(item);
+
/// <summary>
</code_context>
<issue_to_address>
Method name 'GetKeyByITem' contains a capitalization typo.
Please rename to 'GetKeyByItem' for clarity and consistency.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
private object? GetKeyByITem(TItem item) => OnGetRowKey?.Invoke(item);
=======
private object? GetKeyByItem(TItem item) => OnGetRowKey?.Invoke(item);
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `src/BootstrapBlazor/Components/BaseComponents/DynamicElement.cs:128` </location>
<code_context>
builder.AddContent(8, ChildContent);
+ if (Key != null)
+ {
+ builder.SetKey(Key);
+ }
+
</code_context>
<issue_to_address>
Setting the key after adding child content may not have the intended effect.
Call SetKey immediately after opening the element and before AddContent to ensure the key is correctly applied.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/BootstrapBlazor/Components/BaseComponents/DynamicElement.cs
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6527 +/- ##
===========================================
- Coverage 100.00% 99.98% -0.02%
===========================================
Files 714 714
Lines 31397 31405 +8
Branches 4432 4434 +2
===========================================
+ Hits 31397 31399 +2
- Misses 0 4 +4
- Partials 0 2 +2
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:
|
# Conflicts: # src/BootstrapBlazor/BootstrapBlazor.csproj
Link issues
fixes #6526
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Allow explicit key assignment on DynamicElement and enable custom row key generation in Table via the new OnGetRowKey callback.
New Features: