feat(Tab): add OnBeforeShowContextMenu parameter #5737
Merged
Conversation
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
Contributor
Reviewer's Guide by SourceryThis pull request introduces a new Sequence diagram for showing the Tab context menusequenceDiagram
participant Tab Component
participant ContextMenuZone
participant User
User->>Tab Component: Right-clicks on TabItem
Tab Component->>Tab Component: Checks if ShowContextMenu is enabled
alt ShowContextMenu is enabled
Tab Component->>Tab Component: Checks if OnBeforeShowContextMenu is defined
alt OnBeforeShowContextMenu is defined
Tab Component->>Tab Component: Calls OnBeforeShowContextMenu(TabItem)
Tab Component->>User: Waits for OnBeforeShowContextMenu result (bool)
alt Result is true
Tab Component->>ContextMenuZone: OnContextMenu(MouseEventArgs, TabItem)
ContextMenuZone->>User: Displays context menu
else Result is false
Tab Component->>User: Does not display context menu
end
else OnBeforeShowContextMenu is not defined
Tab Component->>ContextMenuZone: OnContextMenu(MouseEventArgs, TabItem)
ContextMenuZone->>User: Displays context menu
end
else ShowContextMenu is disabled
Tab Component->>User: Does not display context menu
end
File-Level Changes
Assessment against 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 @ArgoZhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider extracting the context menu logic into a separate component to improve the Tab component's readability.
- The
OnContextMenumethod inTab.razor.csduplicates logic, consider consolidating it.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
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
This PR enhances the Tab component by introducing a customizable context menu feature including a new OnBeforeShowContextMenu callback, updates to context menu icon parameters, and overall naming consistency improvements. Key changes include:
- Adding new parameters and callbacks for context menu functionality in Tab components.
- Refactoring variable names (e.g., BindPlacement → _bindPlacement) and removing redundant context menu implementations.
- Updating Layout and sample components to integrate the enhanced context menu feature.
Reviewed Changes
Copilot reviewed 7 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTest/Components/LayoutTest.cs | Adds tests for validating context menu icon rendering. |
| src/BootstrapBlazor/Components/Tab/Tab.razor.cs | Introduces new context menu parameters/callback and refactors ContextMenuZone usage. |
| src/BootstrapBlazor/Components/Tab/Tab.razor | Integrates the enhanced context menu markup and event binding. |
| src/BootstrapBlazor/Components/Layout/Layout.razor.cs | Adds the OnBeforeShowContextMenu parameter to Layout. |
| src/BootstrapBlazor/Components/Layout/Layout.razor | Updates context menu integration in Layout. |
| src/BootstrapBlazor.Server/Components/Samples/Tabs.razor.cs | Refactors naming for binding placement and removes obsolete context menu handlers. |
| src/BootstrapBlazor.Server/Components/Samples/Tabs.razor | Updates sample usage to reflect the new context menu parameters. |
Files not reviewed (5)
- src/BootstrapBlazor.Server/Locales/en-US.json: Language not supported
- src/BootstrapBlazor.Server/Locales/zh-CN.json: Language not supported
- src/BootstrapBlazor/Components/Tab/Tab.razor.scss: Language not supported
- src/BootstrapBlazor/Locales/en.json: Language not supported
- src/BootstrapBlazor/Locales/zh.json: Language not supported
Contributor
There was a problem hiding this comment.
Pull Request Overview
The PR introduces a new parameter and callback (OnBeforeShowContextMenu) to the Tabs component for flexible context menu behavior, along with several refactorings and naming improvements across the codebase.
- Added context menu support and a callback in Tabs for conditionally displaying the menu.
- Refactored variable names (e.g. BindPlacement to _bindPlacement) for consistency and simplified method names.
- Updated tests and layout components to integrate the new context menu functionality.
Reviewed Changes
Copilot reviewed 8 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTest/Components/TabTest.cs | Updated context menu test to reflect new UI changes; removed click action verification. |
| test/UnitTest/Components/LayoutTest.cs | Added tests for new icon parameters and OnBeforeShowContextMenu functionality. |
| src/BootstrapBlazor/Components/Tab/Tab.razor.cs | Introduced new context menu parameters and refactored context menu handling logic. |
| src/BootstrapBlazor/Components/Tab/Tab.razor | Updated markup for context menu rendering with new event bindings. |
| src/BootstrapBlazor/Components/Layout/Layout.razor.cs & Layout.razor | Removed redundant context menu handlers and integrated new context menu props. |
| src/BootstrapBlazor.Server/Components/Samples/Tabs.razor.cs & Tabs.razor | Updated binding variable names and cleaned up legacy context menu code. |
Files not reviewed (5)
- src/BootstrapBlazor.Server/Locales/en-US.json: Language not supported
- src/BootstrapBlazor.Server/Locales/zh-CN.json: Language not supported
- src/BootstrapBlazor/Components/Tab/Tab.razor.scss: Language not supported
- src/BootstrapBlazor/Locales/en.json: Language not supported
- src/BootstrapBlazor/Locales/zh.json: Language not supported
Comments suppressed due to low confidence (1)
test/UnitTest/Components/TabTest.cs:52
- [nitpick] Consider adding a test that simulates clicking the context menu item to verify that the associated callback (e.g., OnClick) is actually triggered, ensuring full coverage for the new context menu functionality.
Assert.NotNull(item);
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5737 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 657 657
Lines 29877 29907 +30
Branches 4237 4242 +5
=========================================
+ Hits 29877 29907 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 #5736
Summary By Copilot
This pull request includes significant changes to the
Tabscomponent in theBootstrapBlazorproject. The changes focus on improving the context menu functionality and refactoring code for better readability and maintainability.Context Menu Enhancements:
src/BootstrapBlazor/Components/Tab/Tab.razor,src/BootstrapBlazor/Components/Tab/Tab.razor.cs) [1] [2] [3]OnBeforeShowContextMenuto control the display of the context menu based on the tab item's state. (src/BootstrapBlazor/Components/Tab/Tab.razor.cs)Code Refactoring:
BindPlacementwith_bindPlacementfor better naming consistency. (src/BootstrapBlazor.Server/Components/Samples/Tabs.razor,src/BootstrapBlazor.Server/Components/Samples/Tabs.razor.cs) [1] [2]Tabscomponent code-behind file. (src/BootstrapBlazor.Server/Components/Samples/Tabs.razor.cs) [1] [2]Localization Updates:
src/BootstrapBlazor.Server/Locales/en-US.json,src/BootstrapBlazor.Server/Locales/zh-CN.json) [1] [2]Layout Component Updates:
Layoutcomponent, allowing it to use the same context menu features as theTabscomponent. (src/BootstrapBlazor/Components/Layout/Layout.razor,src/BootstrapBlazor/Components/Layout/Layout.razor.cs) [1] [2]These changes enhance the usability of the
Tabscomponent by providing a more flexible and user-friendly context menu, while also improving the code structure and maintainability.Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add context menu functionality to the Tab component with customizable options and a new OnBeforeShowContextMenu callback
New Features:
Enhancements:
Chores: