Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the EditorForm grouped item aggregation to group by GroupName instead of GroupOrder so that GroupName-based grouping works correctly. Class diagram for updated EditorForm GroupItems grouping behaviorclassDiagram
class EditorForm_TModel_ {
+IEnumerable~KeyValuePair_string_IOrderedEnumerable_IEditorItem__~ GroupItems
-List_IEditorItem_ itemsCache
}
class IEditorItem {
+string GroupName
+int GroupOrder
+int Order
+bool IsVisible(ItemChangedType itemChangedType, bool isSearch)
}
class ItemChangedType
EditorForm_TModel_ --> IEditorItem : uses
IEditorItem --> ItemChangedType : IsVisible parameter
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:
- By switching to GroupBy(GroupName) and ordering by the group key, you’ve removed any use of GroupOrder for group ordering; if the intended behavior is to preserve GroupOrder-based layout while fixing grouping, consider grouping by GroupName but ordering the resulting groups by an appropriate GroupOrder value (e.g., the minimum GroupOrder in each group).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- By switching to GroupBy(GroupName) and ordering by the group key, you’ve removed any use of GroupOrder for group ordering; if the intended behavior is to preserve GroupOrder-based layout while fixing grouping, consider grouping by GroupName but ordering the resulting groups by an appropriate GroupOrder value (e.g., the minimum GroupOrder in each group).Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7568 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 749 749
Lines 32976 32976
Branches 4580 4580
=========================================
Hits 32976 32976
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:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the EditorForm component where only the first group was displayed when multiple groups (GroupName) were defined. The issue was caused by grouping items by GroupOrder instead of GroupName, which caused items with different group names but the same group order value to be merged into a single group.
Changes:
- Changed
GroupItemsproperty to group byGroupNameinstead ofGroupOrder - Version bumped from 10.2.3-beta01 to 10.2.3-beta02
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/BootstrapBlazor/Components/EditorForm/EditorForm.razor.cs | Fixed grouping logic to use GroupName instead of GroupOrder |
| src/BootstrapBlazor/BootstrapBlazor.csproj | Version bump for beta release |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+220
to
221
| .GroupBy(i => i.GroupName).OrderBy(i => i.Key) | ||
| .Select(i => new KeyValuePair<string, IOrderedEnumerable<IEditorItem>>(i.First().GroupName!, i.OrderBy(x => x.Order))); |
There was a problem hiding this comment.
The fix correctly changes the grouping from GroupOrder to GroupName, which resolves the issue where items with different GroupNames were being merged. However, groups are now ordered alphabetically by GroupName (the Key) instead of numerically by GroupOrder. This means the GroupOrder property is no longer respected for determining the display order of groups. Consider changing the ordering to respect GroupOrder while maintaining the GroupName grouping. The correct implementation should order groups by their GroupOrder value, not by their name.
Suggested change
| .GroupBy(i => i.GroupName).OrderBy(i => i.Key) | |
| .Select(i => new KeyValuePair<string, IOrderedEnumerable<IEditorItem>>(i.First().GroupName!, i.OrderBy(x => x.Order))); | |
| .GroupBy(i => i.GroupName).OrderBy(g => g.Min(x => x.GroupOrder)) | |
| .Select(g => new KeyValuePair<string, IOrderedEnumerable<IEditorItem>>(g.First().GroupName!, g.OrderBy(x => x.Order))); |
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 #7566
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Bug Fixes: