feat(Layout): add ShowTabContextMenu parameter #5733
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 tab context menu feature to the No diagrams generated as the changes look simple and do not need a visual representation. 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 adding a
ShowTabContextMenuproperty to the Tab component itself, rather than just the Layout component. - The
OnCloseOthermethod could be simplified by calling_tab.ActiveTab(tabItem)before_tab.CloseOtherTabs().
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 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.
|
|
||
| private string? GetTargetString() => IsFixedTabHeader ? ".tabs-body" : null; | ||
|
|
||
| private async Task OnRefrsh(ContextMenuItem item, object? context) |
Contributor
There was a problem hiding this comment.
nitpick (typo): Typo in method name 'OnRefrsh' – consider renaming to 'OnRefresh'.
Renaming this method will improve consistency and readability for developers working with tab context menu callbacks.
Suggested change
| private async Task OnRefrsh(ContextMenuItem item, object? context) | |
| private async Task OnRefresh(ContextMenuItem item, object? context) |
| namespace UnitTest.Components; | ||
|
|
||
| public class LayoutTest : BootstrapBlazorTestBase | ||
| { |
Contributor
There was a problem hiding this comment.
suggestion (testing): Missing assertions for context menu actions.
Add assertions to verify that the expected actions (refresh, close, close other, close all) occur when the corresponding context menu items are clicked. For example, check if the correct tab is closed or if a refresh action is triggered.
Suggested implementation:
// test context menu onclick event handler
var tab = cut.Find(".tabs-item");
// Test "Refresh" action:
await cut.InvokeAsync(() => tab.ContextMenu());
var buttons = cut.FindAll(".bb-cm-zone > .dropdown-menu .dropdown-item");
var refreshButton = buttons.SingleOrDefault(b => b.TextContent.Contains("Refresh"));
if (refreshButton != null)
{
await cut.InvokeAsync(() => refreshButton.Click());
// Assert that the refresh action occurred.
// Example: the tab remains present and gets a "refreshed" state/class.
Assert.True(cut.Find(".tabs-item").ClassList.Contains("refreshed"), "Expected the tab to be refreshed");
// Re-open the context menu for subsequent actions if needed.
await cut.InvokeAsync(() => tab.ContextMenu());
buttons = cut.FindAll(".bb-cm-zone > .dropdown-menu .dropdown-item");
}
// Test "Close" action:
var closeButton = buttons.SingleOrDefault(b => b.TextContent.Contains("Close") && !b.TextContent.Contains("Other"));
if (closeButton != null)
{
await cut.InvokeAsync(() => closeButton.Click());
// Assert that the tab is closed.
Assert.Throws<ElementNotFoundException>(() => cut.Find(".tabs-item"));
// Re-render or reset the state for further tests as needed.
// (Re-add the tab element if your test requires to run further context menu actions.)
// For example:
// cut.SetParametersAndRender(pb => pb.Add(a => a.TabContextMenuTemplate, tab => b => b.AddContent(0, "test-tab-context-menu")));
// tab = cut.Find(".tabs-item");
}
// Test "Close Other" action:
// (Ensure that at least two tabs are present before testing this behavior.)
await cut.InvokeAsync(() => tab.ContextMenu());
buttons = cut.FindAll(".bb-cm-zone > .dropdown-menu .dropdown-item");
var closeOtherButton = buttons.SingleOrDefault(b => b.TextContent.Contains("Close Other"));
if (closeOtherButton != null)
{
await cut.InvokeAsync(() => closeOtherButton.Click());
// Assert that the "Close Other" action occurred.
// Example: only one tab remains.
Assert.Equal(1, cut.FindAll(".tabs-item").Count);
// Re-render or reset the state for further tests if needed.
}
// Test "Close All" action:
await cut.InvokeAsync(() => tab.ContextMenu());
buttons = cut.FindAll(".bb-cm-zone > .dropdown-menu .dropdown-item");
var closeAllButton = buttons.SingleOrDefault(b => b.TextContent.Contains("Close All"));
if (closeAllButton != null)
{
await cut.InvokeAsync(() => closeAllButton.Click());
// Assert that the "Close All" action occurred.
Assert.Empty(cut.FindAll(".tabs-item"));
}Depending on your codebase, you might need to re-render or reinitialize the tab component between tests for each context menu action.
Also ensure that the assertions (for example, checking class "refreshed" or element absence) match the actual behavior that occurs when these actions are triggered.
If the context menu actions are asynchronous or trigger additional events, you may want to add proper waits or event subscriptions before asserting.
Comment on lines
+54
to
+55
| cut.SetParametersAndRender(pb => pb.Add(a => a.BeforeTabContextMenuTemplate, tab => b => b.AddContent(0, "test-before-tab-context-menu"))); | ||
| cut.Contains("test-before-tab-context-menu"); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Edge case: null BeforeTabContextMenuTemplate.
Add a test case to verify the behavior when BeforeTabContextMenuTemplate is set to null. This ensures the layout renders correctly even without a before template.
Suggested implementation:
using Xunit;[Fact]
public async Task Layout_RendersCorrectly_WhenBeforeTabContextMenuTemplateIsNull()
{
// Arrange: Render the Layout component with BeforeTabContextMenuTemplate set to null.
var cut = Context.RenderComponent<Layout>(pb =>
{
pb.Add(a => a.ToolbarTemplate, builder => builder.AddContent(0, "test-toolbar-template"));
pb.Add(a => a.ShowTabContextMenu, true);
pb.Add(a => a.BeforeTabContextMenuTemplate, (RenderFragment)null);
pb.Add(a => a.TabContextMenuTemplate, tab => b => b.AddContent(0, "test-tab-context-menu"));
});
// Assert: Verify that the toolbar and tab context menu render correctly.
Assert.Contains("test-toolbar-template", cut.Markup);
cut.Contains("bb-cm-zone");
cut.Contains("test-tab-context-menu");
// Ensure that the before tab context menu content is not rendered.
Assert.DoesNotContain("test-before-tab-context-menu", cut.Markup);
// Act: Test the context menu onclick event handler.
var tab = cut.Find(".tabs-item");
await cut.InvokeAsync(() => tab.ContextMenu());
var buttons = cut.FindAll(".bb-cm-zone > .dropdown-menu .dropdown-item");
foreach (var button in buttons)
{
await cut.InvokeAsync(() => button.Click());
}
}If necessary, adjust namespace imports to include Xunit and any other required dependencies. Also, ensure that the test utility Context and the Layout component are correctly imported in this file.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5733 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 657 657
Lines 29833 29877 +44
Branches 4230 4237 +7
=========================================
+ Hits 29833 29877 +44 ☔ 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 #5732
Summary By Copilot
This pull request introduces a new tab context menu feature to the
Layoutcomponent, along with some refactoring and localization updates. The most important changes include adding new parameters and methods to support the context menu, updating icon definitions, and modifying unit tests to cover the new functionality.New Features:
ShowTabContextMenu,BeforeTabContextMenuTemplate,TabContextMenuTemplate,TabContextMenuRefreshIcon,TabContextMenuCloseIcon,TabContextMenuCloseOtherIcon, andTabContextMenuCloseAllIconinLayout.razor.cs. [1] [2] [3]Code Refactoring:
Layout.razorfile to integrate the tab context menu rendering logic. [1] [2]Icon Updates:
ComponentIcons,BootstrapIcons,FontAwesomeIcons, andMaterialDesignIcons. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]Localization:
en.jsonandzh.jsonto include new context menu text entries. [1] [2]Unit Tests:
LayoutTest.csto include tests for the new tab context menu functionality, ensuring the context menu renders correctly and the event handlers work as expected. [1] [2]Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add tab context menu functionality to the Layout component, enabling users to refresh, close, and manage tabs through a right-click context menu
New Features:
Enhancements:
Tests: