Add new Icon parameters to BitNav (#12107)#12110
Add new Icon parameters to BitNav (#12107)#12110msynk merged 3 commits intobitfoundation:developfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds structured icon support to BitNav components by introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Navs/Nav/_BitNavCustomDemo.razor.cs (2)
776-782:⚠️ Potential issue | 🟡 MinorStale property reference:
@item.Icon→@item.Imagein code sample.The inline Razor code sample in
example6RazorCode(line 778) still references@item.Icon, butFoodMenu.Iconwas renamed toFoodMenu.Imagein this PR. The actual Razor template at line 99 of_BitNavCustomDemo.razorcorrectly uses@item.Image.🐛 Fix stale property name in code sample
- <BitIcon IconName=""@item.Icon"" /> + <BitIcon IconName=""@item.Image"" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Navs/Nav/_BitNavCustomDemo.razor.cs` around lines 776 - 782, The Razor code sample stored in example6RazorCode still uses the old property name `@item.Icon`; update that sample to use the renamed property `@item.Image` to match the FoodMenu model and the real template in _BitNavCustomDemo.razor—locate the example6RazorCode string and replace occurrences of "@item.Icon" with "@item.Image" so the sample reflects the current FoodMenu.Image property.
618-618:⚠️ Potential issue | 🟡 MinorInconsistent variable name in code samples:
customCarNavItemsshould becustomFoodNavItems.Several example code strings (e.g.,
example4CsharpCodeline 618,example7CsharpCodeline 939) declareList<FoodMenu> customCarNavItems, but the actual variable iscustomFoodNavItems. This creates a mismatch between the code sample shown to users and the real implementation.Affected code sample strings:
example4CsharpCode,example4RazorCode(lines 585, 595, 607), andexample7CsharpCode(line 939).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Navs/Nav/_BitNavCustomDemo.razor.cs` at line 618, Replace the incorrect sample variable name customCarNavItems with the actual variable name customFoodNavItems inside the example code strings to keep samples consistent with the implementation: update occurrences in the example4CsharpCode, example4RazorCode, and example7CsharpCode string literals so they declare List<FoodMenu> customFoodNavItems (and any references to that identifier inside those example strings) to match the real variable customFoodNavItems used by the component.
🧹 Nitpick comments (1)
src/BlazorUI/Bit.BlazorUI/Extensions/ObjectExtensions.cs (1)
38-43: Implicit operator lookup only checks the target type, not the source type.
targetType.GetMethod("op_Implicit", [value.GetType()])searches forop_Implicitdefined ontargetType. Per C# spec, implicit conversion operators can be defined on either the source or target type. If anop_Implicitis defined onvalue.GetType()instead, this code won't find it.This is fine if all relevant conversions in this codebase are defined on the target type, but worth noting as a limitation. If this becomes a general utility, consider also checking
value.GetType()for implicit operators with a matching return type.♻️ Optional: also check source type for implicit operators
var implicitOp = targetType.GetMethod("op_Implicit", [value.GetType()]); +if (implicitOp is null) +{ + implicitOp = value.GetType().GetMethods(System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Static) + .FirstOrDefault(m => m.Name == "op_Implicit" + && m.ReturnType == targetType + && m.GetParameters().Length == 1 + && m.GetParameters()[0].ParameterType.IsAssignableFrom(value.GetType())); +} if (implicitOp is not null) { var result = implicitOp.Invoke(null, [value]); return result is null ? defaultValue : (T)result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/BlazorUI/Bit.BlazorUI/Extensions/ObjectExtensions.cs` around lines 38 - 43, The code only looks for an implicit operator on targetType (targetType.GetMethod("op_Implicit", [value.GetType()])), so it will miss implicit operators defined on the source type; update the logic to, if no implicitOp is found on targetType, also search value.GetType() for a static "op_Implicit" method whose single parameter matches value.GetType() and whose return type is assignable to targetType (use BindingFlags.Static | BindingFlags.Public and check method.ReturnType.IsAssignableTo(targetType) or equivalent); invoke whichever method you find (implicitOp.Invoke(null, [value])) and cast/return the result the same way as currently done in the block that uses implicitOp and result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razor`:
- Line 16: The current construction embeds the expansion CSS classes only in the
null-coalescing fallback for Nav.ChevronDownIconName, so when
ChevronDownIconName is set the rotation classes are lost; change to always
compute a stateClass (e.g., isExpanded ? "bit-nav-exp" : "bit-ico-r90"), call
BitIconInfo.From using Nav.ChevronDownIcon and Nav.ChevronDownIconName (without
embedding state classes), then append the stateClass to the icon's classes at
render time (e.g., combine chevronIcon.GetCssClasses() with stateClass) so the
expansion/collapse styling is applied regardless of ChevronDownIconName.
In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.parameters.cs`:
- Around line 16-27: Changing BitNav.razor.parameters.cs's ChevronDownIcon from
string? to BitIconInfo? is a breaking API change that mismatches BitNavPanel
which still defines ChevronDownIcon as string? and will break consumers; fix by
either (A) update BitNavPanel to stop passing a string and instead set BitNav's
ChevronDownIconName with the former string value (replace usages of
ChevronDownIcon in BitNavPanel with ChevronDownIconName), or (B) restore
backward-compatibility in BitNav by adding a new [Parameter] string?
ChevronDownIconString (or mark the old string parameter obsolete) that, when
set, maps/constructs the appropriate BitIconInfo or assigns its value to
ChevronDownIconName before render; also update documentation/release
notes/migration guide to show converting ChevronDownIcon="icon-name" to either
ChevronDownIconName="icon-name" (built-in) or ChevronDownIcon="@BitIconInfo.…"
(external).
In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavItem.cs`:
- Around line 56-60: Update the XML <example> in BitNavItem.cs to use consistent
property-assignment syntax for all icon examples (match the style used in
BitNavOption.razor.cs): replace the Bootstrap line that currently uses
attribute-style quotes with a property-assignment form so all three lines read
like Icon = BitIconInfo.Bi("gear-fill"), Icon = BitIconInfo.Fa("solid house"),
and Icon = BitIconInfo.Css("my-icon-class"); ensure the quotes around the inner
string are preserved and the example block compiles as valid XML documentation.
In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cs`:
- Around line 70-74: The XML <example> block in BitNavOption.razor.cs contains
nested double-quotes (e.g., Icon="BitIconInfo.Bi("gear-fill")") which break XML
doc generation; update the examples to match the safe style used in
BitNavItem.cs by either escaping inner quotes with " or converting the
examples to the property-assignment style used in BitNavItem (so there are no
nested unescaped double-quotes), ensuring each example string token (like
"gear-fill", "solid house", "my-icon-class") is represented without breaking the
outer attribute quoting.
In
`@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Navs/Nav/_BitNavCustomDemo.razor.cs`:
- Around line 1189-1197: The Razor code sample stored in example11RazorCode
incorrectly references NavItem members (NavItem.Text, NavItem.ImageName,
NavItem.Url, NavItem.Links, NavItem.Comment); update that string to use the
actual demo model type Section (Section.Text, Section.ImageName, Section.Url,
Section.Links, Section.Comment) so it matches the template used in
_BitNavCustomDemo.razor and the demo model.
In
`@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Navs/Nav/_BitNavItemDemo.razor`:
- Around line 83-93: Update the Font Awesome CDN version used in the demos:
replace the href in the <link rel="stylesheet"
href="https://cdnjs.cloudflare.com/.../css/all.min.css" /> occurrences in the
demo files (look for DemoExample blocks such as the one containing BitNav and
externalIconNavItems in _BitNavItemDemo.razor and matching entries in
_BitNavOptionDemo.razor, _BitNavItemDemo.razor.cs, _BitNavOptionDemo.razor.cs)
to reference the current latest patch (7.2.0) so all four demo files use the
same up-to-date CDN URL.
In
`@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Navs/Nav/_BitNavOptionDemo.razor.cs`:
- Around line 420-434: The example11RazorCode verbatim string has an extra level
of quote escaping (e.g., """"fa-solid fa-house"""") which yields invalid
Razor/C# output; update the string so all BitIconInfo.Css(...) and
BitIconInfo.Fa(...) usages inside example11RazorCode use two double-quotes
around the CSS/class values (e.g., ""fa-solid fa-house"") instead of four, i.e.
replace each """"..."""" with ""..."" so the rendered Razor attributes are
valid.
---
Outside diff comments:
In
`@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Navs/Nav/_BitNavCustomDemo.razor.cs`:
- Around line 776-782: The Razor code sample stored in example6RazorCode still
uses the old property name `@item.Icon`; update that sample to use the renamed
property `@item.Image` to match the FoodMenu model and the real template in
_BitNavCustomDemo.razor—locate the example6RazorCode string and replace
occurrences of "@item.Icon" with "@item.Image" so the sample reflects the
current FoodMenu.Image property.
- Line 618: Replace the incorrect sample variable name customCarNavItems with
the actual variable name customFoodNavItems inside the example code strings to
keep samples consistent with the implementation: update occurrences in the
example4CsharpCode, example4RazorCode, and example7CsharpCode string literals so
they declare List<FoodMenu> customFoodNavItems (and any references to that
identifier inside those example strings) to match the real variable
customFoodNavItems used by the component.
---
Nitpick comments:
In `@src/BlazorUI/Bit.BlazorUI/Extensions/ObjectExtensions.cs`:
- Around line 38-43: The code only looks for an implicit operator on targetType
(targetType.GetMethod("op_Implicit", [value.GetType()])), so it will miss
implicit operators defined on the source type; update the logic to, if no
implicitOp is found on targetType, also search value.GetType() for a static
"op_Implicit" method whose single parameter matches value.GetType() and whose
return type is assignable to targetType (use BindingFlags.Static |
BindingFlags.Public and check method.ReturnType.IsAssignableTo(targetType) or
equivalent); invoke whichever method you find (implicitOp.Invoke(null, [value]))
and cast/return the result the same way as currently done in the block that uses
implicitOp and result.
closes #12107
Summary by CodeRabbit
Release Notes
New Features
Iconparameter to Navigation components supportingBitIconInfoobjects for flexible icon rendering.ChevronDownIconparameter to BitNav component.IconoverIconNamewhen both are specified.Documentation