feat(Tab): add ShowContextFullScreenButton parameter #5739
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 Tab context menu full screen togglesequenceDiagram
participant User
participant Tab Component
participant Context Menu
participant FullScreenService
User->>Tab Component: Right-clicks on TabItem
Tab Component->>Context Menu: Shows context menu with Full Screen option
User->>Context Menu: Clicks Full Screen
Context Menu->>Tab Component: Calls OnFullScreen
Tab Component->>FullScreenService: ToggleById(TabItem ID)
FullScreenService->>FullScreenService: Toggles full screen mode for TabItem
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds a full-screen option to the Tab component’s context menu while updating related icon mappings, localization files, and tests.
- Introduces the ShowContextMenuFullScreen parameter and corresponding OnFullScreen handler in the Tab component.
- Adds icon definitions for full-screen support in multiple icon sets and enum mappings.
- Updates tests and sample files to reflect the new functionality and removes the deprecated IsDisabled attribute from TabItem.
Reviewed Changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTest/Components/TabTest.cs | Adds a test case to enable ShowContextMenuFullScreen and still sets IsDisabled on TabItem. |
| src/BootstrapBlazor/Icons/MaterialDesignIcons.cs | Adds a full-screen icon mapping for Material Design icons. |
| src/BootstrapBlazor/Icons/FontAwesomeIcons.cs | Adds a full-screen icon mapping for FontAwesome icons. |
| src/BootstrapBlazor/Icons/BootstrapIcons.cs | Adds a full-screen icon mapping for Bootstrap icons. |
| src/BootstrapBlazor/Enums/ComponentIcons.cs | Introduces a new enum value for the full-screen icon. |
| src/BootstrapBlazor/Components/Tab/Tab.razor.cs | Introduces the new ShowContextMenuFullScreen parameter, adds the full-screen icon, injects FullScreenService, and updates GetIdByTabItem. |
| src/BootstrapBlazor/Components/Tab/Tab.razor | Updates the context menu to conditionally render the full-screen option when enabled. |
| src/BootstrapBlazor.Server/Components/Samples/Tabs.razor | Updates sample usage of the Tab component to align with the updated API. |
Files not reviewed (3)
- src/BootstrapBlazor/Components/ContextMenu/ContextMenu.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 (2)
src/BootstrapBlazor/Components/Tab/Tab.razor.cs:1036
- Changing the return type from nullable to non-nullable assumes that ComponentIdGenerator.Generate always returns a non-null string. Verify that this assumption holds to avoid potential null reference exceptions.
private string GetIdByTabItem(TabItem item) => ComponentIdGenerator.Generate(item);
test/UnitTest/Components/TabTest.cs:39
- The test case is still setting the 'IsDisabled' property on TabItem, but this property has been removed from the production code. Consider updating the test to reflect the current implementation.
pb.Add(a => a.IsDisabled, true);
![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
ShowFullScreenparameter to theTabItemcomponent to allow individual tabs to control the full-screen option.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue 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.
| pb.AddChildContent<Tab>(pb => | ||
| { | ||
| pb.Add(a => a.ShowContextMenu, true); | ||
| pb.Add(a => a.ShowContextMenuFullScreen, true); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Test both true and false values for ShowContextMenuFullScreen
It's important to test both true and false values for the ShowContextMenuFullScreen parameter to ensure that the full-screen button is correctly shown or hidden in the context menu.
Suggested implementation:
pb.AddChildContent<Tab>(pb =>
{
pb.Add(a => a.ShowContextMenu, true);
pb.Add(a => a.ShowContextMenu, true);
pb.Add(a => a.ShowContextMenuFullScreen, true);
pb.AddChildContent<TabItem>(pb =>
{
pb.Add(a => a.IsDisabled, true);
});
});
// Additional test case to validate ShowContextMenuFullScreen with false value
pb.AddChildContent<Tab>(pb =>
{
pb.Add(a => a.ShowContextMenu, true);
pb.Add(a => a.ShowContextMenuFullScreen, false);
pb.AddChildContent<TabItem>(pb =>
{
pb.Add(a => a.IsDisabled, true);
});
});The additional test case will allow you to verify that the full-screen button is correctly hidden when ShowContextMenuFullScreen is false. Adjust assertions (if any) in your test framework accordingly to validate the UI behavior.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5739 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 657 657
Lines 29907 29923 +16
Branches 4242 4245 +3
=========================================
+ Hits 29907 29923 +16 ☔ 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 #5738
Summary By Copilot
This pull request introduces several enhancements and bug fixes to the
BootstrapBlazorproject, focusing on theTabcomponent, context menu styling, and localization updates. The most important changes include adding a full-screen option to the tab context menu, updating the context menu styles, and modifying localization files to support the new full-screen option.Enhancements to the
Tabcomponent:ShowContextMenuFullScreento control the visibility of the full-screen button in the context menu, with a default value oftrue.OnFullScreento handle the full-screen toggle action in the context menu.FullScreenServiceto manage full-screen actions.TabContextMenuFullScreenIconto represent the full-screen action in the context menu. [1] [2] [3] [4]Context menu styling updates:
.dividerclass inContextMenu.razor.scssto improve spacing.Localization updates:
Bug fixes:
IsDisabledattribute from theTabIteminTabs.razorto ensure proper functionality.GetIdByTabItemmethod by removing the nullable return type.Testing updates:
ShowContextMenuFullScreenparameter inTabTest.cs.Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add full-screen option to Tab component's context menu with new configuration parameters and service integration
New Features:
ShowContextMenuFullScreenparameter to control the visibility of the full-screen button in the Tab component's context menuBug Fixes:
IsDisabledattribute fromTabItemto ensure proper functionalityGetIdByTabItemmethod by removing nullable return typeEnhancements:
ContextMenuFullScreenIconparameter to customize the full-screen iconFullScreenServiceto manage full-screen actions for tabsTests:
ShowContextMenuFullScreenparameter