Improve Iconography page (#12576)#12577
Open
mhrastegari wants to merge 6 commits into
Open
Conversation
WalkthroughAdds a reusable ChangesIcon details panel feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant IconographyPage
participant HttpClient
participant IconDetailRow
User->>IconographyPage: click icon tile
IconographyPage->>IconographyPage: OpenIconPanel(IconInfo)
IconographyPage->>HttpClient: fetch icons CSS when needed
HttpClient-->>IconographyPage: return parsed glyph mappings
IconographyPage-->>IconDetailRow: render detail rows
User->>IconDetailRow: click copy button
IconDetailRow->>IconographyPage: OnCopy(Value, CopyKey)
IconographyPage->>IconographyPage: CopyToClipboard(text, feedbackKey)
IconographyPage-->>IconDetailRow: update CopyFeedbackKey
IconDetailRow-->>User: show copied state
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Components/IconDetailRow.razor.scss`:
- Around line 17-26: The IconDetailRow.razor.scss rule for
.icon-detail-row-value uses multi-word font-family names without quotes, which
Stylelint flags. Update the font-family declaration in the
.icon-detail-row-value block so any family names containing spaces are quoted,
while keeping the same fallback stack and monospace intent intact.
In
`@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor.cs`:
- Around line 9-18: The RazorIconInfo property in IconInfo is producing
copy-paste snippets with invalid Razor attribute quoting because the nested
quotes around BitIconInfo.Bit(Value) are not escaped for the outer double-quoted
attribute. Update RazorIconInfo so the generated string is valid Razor syntax
when pasted into a .razor file, and verify the resulting snippet still matches
the style used by ConstantReference, CssClass, and RazorIconName.
- Around line 92-102: The icon panel is clearing selectedIcon too early, which
can blank the panel body before BitPanel finishes its close animation. Update
CloseIconPanel and its call path from HandleIconPanelDismissed so selectedIcon
is only cleared after the panel has fully dismissed, while still setting
isIconPanelOpen to false immediately. Use the existing IconographyPage component
state flow to defer the content reset until after the close transition
completes.
In
`@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor.scss`:
- Around line 14-18: The .icon-name rule uses the deprecated word-break:
break-word value, so update the IconographyPage.razor.scss styles to use a
current text-wrapping approach instead. Locate the .icon-name selector and
replace the deprecated declaration with the appropriate modern CSS
property/value combination that preserves label wrapping behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 21b8f43a-f81e-497b-8090-1a455232d70b
📒 Files selected for processing (6)
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Components/IconDetailRow.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Components/IconDetailRow.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Components/IconDetailRow.razor.scsssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor.scss
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
Member
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor (1)
12-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate stale
PageOutletdescription to reflect the new details panel behavior.The description still says "copy any icon name with a click," but the click now opens an icon details panel. The in-page note (lines 44–45) was updated; this should be too.
✏️ Proposed fix
- Description="Browse and search the full set of Fabric (MDL2) icons available in bit BlazorUI, and copy any icon name with a click." /> + Description="Browse and search the full set of Fabric (MDL2) icons available in bit BlazorUI. Click any icon to view its details and copy usage snippets." />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor` around lines 12 - 14, Update the Description attribute of the IconographyPage PageOutlet to describe that clicking an icon opens its details panel, replacing the stale claim that users can copy the icon name with a click.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor.cs`:
- Around line 42-47: Remove the unused cascadingParams field from the
IconographyPage code-behind; the Razor markup already references
_cascadingParams defined in its `@code` block, so do not alter the existing markup
or that field.
- Around line 111-118: The GetGlyphCode method currently formats the literal
backslash from CSS-escaped glyph values instead of the actual codepoint. Decode
or remove the leading backslash from the glyph string before calling
char.ConvertToUtf32, then format the resulting codepoint with the existing
escape prefix and hexadecimal formatting.
---
Outside diff comments:
In
`@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor`:
- Around line 12-14: Update the Description attribute of the IconographyPage
PageOutlet to describe that clicking an icon opens its details panel, replacing
the stale claim that users can copy the icon name with a click.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a12bad27-7c83-4281-8a74-ea0d515995de
📒 Files selected for processing (6)
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Components/IconDetailRow.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Components/IconDetailRow.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Components/IconDetailRow.razor.scsssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor.scss
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor.cs (1)
111-118: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
GetGlyphCodeproduces incorrect glyph codes — it formats the CSS backslash, not the codepoint.CSS escape values captured by the regex (e.g.,
\E498) start with a literal backslash.char.ConvertToUtf32(glyph, 0)reads that backslash character (U+005C = 92), so the output becomes\005Cinstead of\E498. This was flagged in a prior review and remains unaddressed.🐛 Proposed fix
private string? GetGlyphCode(IconInfo icon) { if (iconGlyphs is null) return null; if (iconGlyphs.TryGetValue(icon.Value, out var glyph) is false || string.IsNullOrEmpty(glyph)) return null; - return $"\\{char.ConvertToUtf32(glyph, 0):X4}"; + var hex = glyph.TrimStart('\\'); + return int.TryParse(hex, System.Globalization.NumberStyles.HexNumber, null, out var code) + ? $"\\{code:X4}" + : glyph; }#!/bin/bash # Verify the CSS glyph format to confirm the backslash issue. scss=$(fd -t f "bit.blazorui.icons.scss" src/ 2>/dev/null | head -1) if [ -n "$scss" ]; then echo "== Sample content declarations from SCSS source ==" rg -n 'content:' "$scss" | head -10 echo "" echo "== Checking for backslash-escape sequences ==" rg -qP 'content:\s*"\\' "$scss" && echo "CSS uses \\HEX escape sequences (confirms bug)" || echo "No backslash escapes found" else echo "SCSS source not found; searching for compiled CSS..." fd -t f "bit.blazorui.icons.css" 2>/dev/null | head -5 fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor.cs` around lines 111 - 118, The GetGlyphCode method incorrectly converts the leading CSS backslash instead of the glyph codepoint. Update GetGlyphCode to strip the leading backslash from glyph before parsing or returning the escape, then preserve the actual hexadecimal codepoint (such as E498) with a single CSS backslash prefix; retain the existing null and empty-value handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor.cs`:
- Around line 111-118: The GetGlyphCode method incorrectly converts the leading
CSS backslash instead of the glyph codepoint. Update GetGlyphCode to strip the
leading backslash from glyph before parsing or returning the escape, then
preserve the actual hexadecimal codepoint (such as E498) with a single CSS
backslash prefix; retain the existing null and empty-value handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a12bad27-7c83-4281-8a74-ea0d515995de
📒 Files selected for processing (6)
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Components/IconDetailRow.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Components/IconDetailRow.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Components/IconDetailRow.razor.scsssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor.scss
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
Member
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor (1)
14-14: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the stale
PageOutletdescription.The description still says "copy any icon name with a click," but clicking an icon now opens a details panel. Update to reflect the new behavior.
✏️ Proposed fix
- Description="Browse and search the full set of Fabric (MDL2) icons available in bit BlazorUI, and copy any icon name with a click." /> + Description="Browse and search the full set of Fabric (MDL2) icons available in bit BlazorUI. Click an icon to preview sizes, colors, and copy usage snippets." />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor` at line 14, Update the PageOutlet Description in IconographyPage to state that clicking an icon opens its details panel instead of claiming that it copies the icon name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor`:
- Line 14: Update the PageOutlet Description in IconographyPage to state that
clicking an icon opens its details panel instead of claiming that it copies the
icon name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0d8f6049-59e0-4e9b-91d5-245e1925b2dc
📒 Files selected for processing (6)
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Components/IconDetailRow.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Components/IconDetailRow.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Components/IconDetailRow.razor.scsssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/IconographyPage.razor.scss
msynk
changed the title
Iconography page (#12576)
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Close #12576
Summary by CodeRabbit