Fix and improve position feature of BitPivot (#11421) #11425
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 WalkthroughEnum members Left/Right are renamed to Start/End for BitPivot, with corresponding CSS class changes and RTL-aware logical properties in styles. Tests and demos are updated to use Start/End. A minor whitespace edit appears in BitPivot.razor. Demo examples expand to showcase Start/End, including RTL scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/BlazorUI/Bit.BlazorUI/Components/Navs/Pivot/BitPivot.scss (1)
88-95: Limit hover expansion to Top/Bottom to avoid Start/End indicator distortion
.bit-pvt-lnk .bit-pvti-sel:hover::before { inset-inline: 0 }expands the horizontal indicator correctly for Top/Bottom, but when Position is Start/End (vertical indicator), this rule stretches the indicator across the full width and conflicts with the vertical animation block below.Scope the hover expansion to Top/Bottom only and remove the generic rule.
Apply:
.bit-pvt-lnk { .bit-pvti-sel { @media(hover:hover) { &:hover { - &::before { - inset-inline: 0; - } + // moved to Top/Bottom scope to avoid Start/End conflicts } } } } + +// Only Top/Bottom should expand inline on hover +.bit-pvt-top, +.bit-pvt-btm { + &.bit-pvt-lnk, // headerType: Link + .bit-pvt-lnk { // in case classes are combined in different orders + .bit-pvti-sel { + @media(hover:hover) { + &:hover { + &::before { inset-inline: 0; } + } + } + } + } +}src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Navs/Pivot/BitPivotDemo.razor (1)
462-497: Fix leftover Left/Right usages in Pivot Position demo
- Replace BitPivotPosition.Left/Right in src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Navs/Pivot/BitPivotDemo.razor.cs (example10RazorCode defined ~line 812; occurrences at ~lines 881 and 915) and update the demo page (src/.../BitPivotDemo.razor, RazorCode at ~line 387) and any accompanying doc/description strings to use BitPivotPosition.Start/End and "Start"/"End" wording.
🧹 Nitpick comments (6)
src/BlazorUI/Bit.BlazorUI/Components/Navs/Pivot/BitPivot.scss (1)
56-70: Optional: prefer gap/logical margins for vertical layout spacingWhen Position is Start/End, items stack vertically but
.bit-pvtistill usesmargin-right. Consider usinggapon.bit-pvt-hctormargin-block-endon items to achieve consistent spacing in vertical layout.Also applies to: 226-246
src/BlazorUI/Bit.BlazorUI/Components/Navs/Pivot/BitPivotPosition.cs (1)
16-24: Add Left/Right enum aliases marked Obsolete to preserve source/binary compatibilityRenaming public enum members is a breaking change. You can keep compatibility by adding aliases and marking them
[Obsolete]while keeping the newStart/Endas canonical.Apply:
public enum BitPivotPosition { /// <summary> /// Display header at the top. /// </summary> Top, /// <summary> /// Display header at the bottom. /// </summary> Bottom, /// <summary> /// Display header at the start (Left for LTR and Right for RTL). /// </summary> Start, /// <summary> /// Display header at the end (Right for LTR and Left for RTL). /// </summary> End, + + /// <summary> + /// Obsolete: use Start (Left for LTR and Right for RTL). + /// </summary> + [Obsolete("BitPivotPosition.Left is obsolete. Use BitPivotPosition.Start instead.")] + Left = Start, + + /// <summary> + /// Obsolete: use End (Right for LTR and Left for RTL). + /// </summary> + [Obsolete("BitPivotPosition.Right is obsolete. Use BitPivotPosition.End instead.")] + Right = End, }src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Navs/Pivot/BitPivotDemo.razor.cs (1)
423-431: Docs OK for Start/End; update earlier sample that still shows Left/RightEnum descriptions here are correct. The earlier
example10RazorCodestill demonstratesBitPivotPosition.Left/Right, which no longer exist and can confuse users. Please update that sample toStart/End.Suggested replacement for the two blocks in example10:
private readonly string example10RazorCode = @" <BitPivot Position=""BitPivotPosition.Top""> ... </BitPivot> <BitPivot Position=""BitPivotPosition.Bottom""> ... </BitPivot> <BitPivot Position=""BitPivotPosition.Start""> ... </BitPivot> <BitPivot Position=""BitPivotPosition.End""> ... </BitPivot>";src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Navs/Pivot/BitPivotDemo.razor (3)
680-697: Add lang for RTL a11y.Mark Persian content language for screen readers.
Apply:
- <div class="box"> + <div class="box" lang="fa">
699-724: RTL + Start: add lang and align OverflowBehavior with first RTL sample.Improves a11y and keeps behavior consistent when headers overflow.
Apply:
- <div class="box"> + <div class="box" lang="fa"> @@ - <BitPivot Dir="BitDir.Rtl" Position="BitPivotPosition.Start"> + <BitPivot Dir="BitDir.Rtl" Position="BitPivotPosition.Start" OverflowBehavior="@BitPivotOverflowBehavior.Scroll">
726-751: RTL + End: mirror the same tweaks.Add lang and OverflowBehavior for parity with other RTL blocks.
Apply:
- <div class="box"> + <div class="box" lang="fa"> @@ - <BitPivot Dir="BitDir.Rtl" Position="BitPivotPosition.End"> + <BitPivot Dir="BitDir.Rtl" Position="BitPivotPosition.End" OverflowBehavior="@BitPivotOverflowBehavior.Scroll">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (7)
src/BlazorUI/Bit.BlazorUI.Tests/Components/Navs/Pivot/BitPivotTests.cs(2 hunks)src/BlazorUI/Bit.BlazorUI/Components/Navs/Pivot/BitPivot.razor(0 hunks)src/BlazorUI/Bit.BlazorUI/Components/Navs/Pivot/BitPivot.razor.cs(1 hunks)src/BlazorUI/Bit.BlazorUI/Components/Navs/Pivot/BitPivot.scss(4 hunks)src/BlazorUI/Bit.BlazorUI/Components/Navs/Pivot/BitPivotPosition.cs(1 hunks)src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Navs/Pivot/BitPivotDemo.razor(3 hunks)src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Navs/Pivot/BitPivotDemo.razor.cs(2 hunks)
💤 Files with no reviewable changes (1)
- src/BlazorUI/Bit.BlazorUI/Components/Navs/Pivot/BitPivot.razor
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build and test
🔇 Additional comments (3)
src/BlazorUI/Bit.BlazorUI.Tests/Components/Navs/Pivot/BitPivotTests.cs (1)
71-76: LGTM: tests updated to Start/End and class mappingsCoverage for Top/Bottom/Start/End CSS class selection looks correct and aligned with the new enum values and SCSS class names.
Also applies to: 84-96
src/BlazorUI/Bit.BlazorUI/Components/Navs/Pivot/BitPivot.razor.cs (1)
122-129: LGTM: Position mapping updated to sta/end — verify no stale lft/rgt referencesStart -> bit-pvt-sta and End -> bit-pvt-end align with SCSS; default remains Top. rg returned no output; re-run a repo-wide search for 'bit-pvt-(lft|rgt)' or explicitly check 'bit-pvt-lft' and 'bit-pvt-rgt' to confirm no stale mappings remain.
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Navs/Pivot/BitPivotDemo.razor (1)
499-535: End mapping LGTM.Consistent with logical inline-end semantics. Same repo-wide Left/Right sweep recommended.
Use the same script in the prior comment.
.../Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Navs/Pivot/BitPivotDemo.razor.cs
Show resolved
Hide resolved
1 participant
closes #11421
Summary by CodeRabbit
New Features
Refactor
Style
Documentation
Tests