feat(Tab): add ToolbarTemplate parameter #5694
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 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 sample that demonstrates the
ToolbarTemplatefunctionality with interactive elements. - The new parameters should be added to the component's documentation page.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues 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.
Comment on lines
+71
to
+72
| cut.Contains("test-refresh-icon"); | ||
| cut.Contains("test-refresh-tooltip-text"); |
Contributor
There was a problem hiding this comment.
issue (testing): Missing assertion for FullscreenToolbarButton
The test for ToolbarTooltipText_Ok is missing assertions for the fullscreen button's icon and tooltip text. Please add cut.Contains("test-fullscreen-icon"); and cut.Contains("test-fullscreen-tooltip-text"); to ensure these properties are also rendered correctly.
Comment on lines
+28
to
+29
| [Fact] | ||
| public void ToolbarTemplate_Ok() |
Contributor
There was a problem hiding this comment.
suggestion (testing): Missing test cases for null/empty ToolbarTemplate
It would be beneficial to add tests to verify the behavior when ToolbarTemplate is null or empty. This ensures the component handles these scenarios gracefully and doesn't throw exceptions or render unexpected content.
Suggested implementation:
[Fact]
public void ToolbarTemplate_Null()
{
// Test behavior when ToolbarTemplate is not provided (i.e., null)
var cut = Context.RenderComponent<Tab>(pb =>
{
pb.AddChildContent<TabItem>(pb =>
{
pb.Add(a => a.Text, "Tab1");
pb.Add(a => a.Url, "/Index");
pb.Add(a => a.Closable, true);
pb.Add(a => a.Icon, "fa-solid fa-font-awesome");
pb.Add(a => a.ChildContent, "Tab1-Content");
});
// ToolbarTemplate intentionally omitted to simulate null value.
});
// Assert the component renders correctly without the toolbar content.
// Adjust the assertion as needed based on the actual expected markup.
Assert.DoesNotContain("test-toolbar-template", cut.Markup);
}
[Fact]
public void ToolbarTemplate_Empty()
{
// Test behavior when ToolbarTemplate is provided but empty.
var cut = Context.RenderComponent<Tab>(pb =>
{
pb.AddChildContent<TabItem>(pb =>
{
pb.Add(a => a.Text, "Tab1");
pb.Add(a => a.Url, "/Index");
pb.Add(a => a.Closable, true);
pb.Add(a => a.Icon, "fa-solid fa-font-awesome");
pb.Add(a => a.ChildContent, "Tab1-Content");
});
// Provide an empty template.
pb.Add(a => a.ToolbarTemplate, builder => { });
});
// Assert that the rendered markup does not contain any unintended toolbar content.
Assert.DoesNotContain("test-toolbar-template", cut.Markup);
}Depending on how the Tab component is expected to behave, you may need to adjust the assertions to check for specific markup conditions.
| Type = "RenderFragment", | ||
| ValueList = " — ", | ||
| DefaultValue = " — " | ||
| }, |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider creating a helper method to construct the attribute definitions to reduce repetition and improve maintainability of the code.
To reduce repetition and improve maintainability, consider extracting a helper method to construct these attribute definitions. For example, you could create a helper that takes the specific parameters and returns the new attribute instance:
private AttributeItem CreateAttributeItem(string name, string descriptionKey, string type, string valueList, string defaultValue)
{
return new AttributeItem
{
Name = name,
Description = Localizer[descriptionKey].Value,
Type = type,
ValueList = valueList,
DefaultValue = defaultValue
};
}Then, you can simplify the array initialization:
var attributes = new[]
{
CreateAttributeItem(nameof(Tab.ShowToolbar), "AttributeShowToolbar", "bool", "true|false", "false"),
CreateAttributeItem(nameof(Tab.ToolbarTemplate), "AttributeToolbarTemplate", "RenderFragment", " — ", " — "),
CreateAttributeItem(nameof(Tab.ShowRefreshToolbarButton), "AttributeShowRefreshToolbarButton", "bool", "true|false", "true"),
CreateAttributeItem(nameof(Tab.ShowFullscreenToolbarButton), "AttributeShowFullscreenToolbarButton", "bool", "true|false", "true"),
CreateAttributeItem(nameof(Tab.RefreshToolbarTooltipText), "AttributeRefreshToolbarTooltipText", "string?", " — ", " — "),
CreateAttributeItem(nameof(Tab.FullscreenToolbarTooltipText), "AttributeFullscreenToolbarTooltipText", "string?", " — ", " — "),
CreateAttributeItem(nameof(Tab.RefreshToolbarButtonIcon), "AttributeRefreshToolbarButtonIcon", "string?", " — ", " — "),
CreateAttributeItem(nameof(Tab.FullscreenToolbarButtonIcon), "AttributeFullscreenToolbarButtonIcon", "string?", " — ", " — ")
};This approach retains all functionality and significantly reduces the duplicated patterns in your code.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5694 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 657 657
Lines 29776 29789 +13
Branches 4220 4223 +3
=========================================
+ Hits 29776 29789 +13 ☔ 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 #5693
Summary By Copilot
This pull request introduces several changes to the
Tabscomponent in theBootstrapBlazorproject, focusing on adding new toolbar functionalities and enhancing existing tab styles. The most important changes include adding toolbar templates, updating localization files, and modifying the tab component to support new toolbar features.Enhancements to Tab Toolbar:
ToolbarTemplate,ShowToolbar,ShowRefreshToolbarButton,ShowFullscreenToolbarButton,RefreshToolbarTooltipText, andFullscreenToolbarTooltipTextto theTabcomponent to support toolbar customization. (src/BootstrapBlazor/Components/Tab/Tab.razor.cs- [1] [2] [3]Tabcomponent to include the new toolbar template and buttons with tooltip support. (src/BootstrapBlazor/Components/Tab/Tab.razor- [1] [2]TabToolbarButtoncomponent to support tooltips and handle click events. (src/BootstrapBlazor/Components/Tab/TabToolbarButton.razor- [1]src/BootstrapBlazor/Components/Tab/TabToolbarButton.razor.cs- [2] [3]Localization Updates:
src/BootstrapBlazor.Server/Locales/en-US.json- [1]src/BootstrapBlazor.Server/Locales/zh-CN.json- [2]src/BootstrapBlazor/Locales/en.json- [3]src/BootstrapBlazor/Locales/zh.json- [4]Other Changes:
Tabs.razorsample to demonstrate the new toolbar features and tab styles. (src/BootstrapBlazor.Server/Components/Samples/Tabs.razor- [1] [2] [3] [4]src/BootstrapBlazor/Components/Tab/Tab.razor.scss- src/BootstrapBlazor/Components/Tab/Tab.razor.scssR674-R679)Tabcomponent instance to templates. (src/BootstrapBlazor/Components/Tab/Tab.razor- [1] [2]These changes collectively enhance the functionality and customization options of the
Tabscomponent, making it more versatile and user-friendly.Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Adds a ToolbarTemplate parameter to the Tab component, allowing users to customize the toolbar with custom content. It also adds ShowRefreshToolbarButton and ShowFullscreenToolbarButton parameters to control the visibility of the refresh and fullscreen buttons, and provides localization support for toolbar-related text.
New Features:
Tests: