feat(preview-modernization): Update Sidebar Nav icons and tooltips #4373
Conversation
WalkthroughFeature-flag-enabled modernization of sidebar navigation components introduces Blueprint tooltips and visual enhancements with SVG icon scaling on hover/focus states across multiple components. Changes
Sequence DiagramsequenceDiagram
participant Component as Sidebar Component
participant FeatureFlag as useFeatureConfig<br/>(previewModernization)
participant Modern as Modernized Path<br/>(BPTooltip)
participant Legacy as Legacy Path<br/>(Tooltip)
participant UI as Rendered UI
Component->>FeatureFlag: Check feature flag
alt Feature Enabled
FeatureFlag-->>Component: true
Component->>Modern: Render with BPTooltip
Modern->>Modern: Apply modernized CSS classes
Modern->>UI: Render button with Blueprint tooltip<br/>+ SVG scaling on hover/focus
else Feature Disabled
FeatureFlag-->>Component: false
Component->>Legacy: Render with legacy Tooltip
Legacy->>UI: Render button with legacy tooltip<br/>+ existing styling
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 0
🧹 Nitpick comments (3)
src/elements/content-sidebar/SidebarNavSignButton.scss (1)
14-14: Optional: Remove semicolon after comment.The semicolon after the inline comment is unusual and redundant in CSS.
Apply this diff if you'd like to clean it up:
- margin-top: 2px; // for visual alignment; + margin-top: 2px; // for visual alignmentsrc/elements/content-sidebar/SidebarNavSignButton.tsx (1)
4-7: Modernization and tooltip behavior look good; handlearia-expandedmore robustlyThe feature-flagged BPTooltip path, reusing
PlainButtonand skipping the inner tooltip when FTUX is active, matches the legacy behavior while integrating Blueprint tooltips. One small improvement:isDropdownOpenonly treats'aria-expanded'as the string'true', but callers may pass a boolean.You can make this more robust without changing behavior:
- const renderButtonWithTooltip = () => { - const isDropdownOpen = rest['aria-expanded'] === 'true'; + const renderButtonWithTooltip = () => { + const ariaExpanded = rest['aria-expanded']; + const isDropdownOpen = ariaExpanded === true || ariaExpanded === 'true';This keeps the color logic correct regardless of whether
aria-expandedis passed as a boolean or a string.Also applies to: 13-13, 37-38, 53-80, 82-97, 99-107
src/elements/content-sidebar/SidebarNavButton.js (1)
11-18: Dynamic tooltip selection andisActivecloning are sound; avoid leakingisActiveto DOM elementsUsing
TooltipComponentto switch between BPTooltip and the legacy Tooltip based onisPreviewModernizationEnabled, with matchingtooltipProps, keeps behavior consistent across the routerDisabled and Route paths. Cloningchildrento injectisActiveis a nice way to let icon wrappers react to active state, but doing this unconditionally can push anisActiveattribute down to raw DOM elements if a child ultimately renders, for example, a<span>or<svg>and blindly spreads props.You can keep the feature while avoiding unknown DOM attributes by only injecting
isActivefor non-DOM React components:- // Clone children and pass isActive prop - const childrenWithProps = React.isValidElement(children) - ? React.cloneElement(children, { isActive: isActiveValue }) - : children; + // Clone children and pass isActive prop only for non-DOM components + const childrenWithProps = + React.isValidElement(children) && typeof children.type !== 'string' + ? React.cloneElement(children, { isActive: isActiveValue }) + : children;Apply the same pattern in both the
routerDisabledandRoutebranches.Also, the
import typeusage forInternalSidebarNavigation,InternalSidebarNavigationHandler, andViewTypeValuesis valid Flow syntax for this file (@flowheader); Biome’s “TypeScript-only” warning here is a tooling configuration issue rather than a code problem.Also applies to: 34-35, 56-61, 68-75, 92-99, 93-95, 144-151, 145-147
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/components/sidebar-toggle-button/SidebarToggleButton.js(2 hunks)src/components/sidebar-toggle-button/SidebarToggleButton.scss(1 hunks)src/elements/content-sidebar/SidebarNav.js(8 hunks)src/elements/content-sidebar/SidebarNavButton.js(6 hunks)src/elements/content-sidebar/SidebarNavButton.scss(3 hunks)src/elements/content-sidebar/SidebarNavSignButton.scss(1 hunks)src/elements/content-sidebar/SidebarNavSignButton.tsx(4 hunks)src/elements/content-sidebar/_mixins.scss(2 hunks)src/elements/content-sidebar/additional-tabs/AdditionalTab.scss(1 hunks)src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js(2 hunks)src/elements/content-sidebar/additional-tabs/AdditionalTabs.js(3 hunks)src/elements/content-sidebar/additional-tabs/AdditionalTabs.scss(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-09-03T18:10:42.760Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarNav.js:195-206
Timestamp: 2025-09-03T18:10:42.760Z
Learning: In src/elements/content-sidebar/SidebarNav.js, the maintainer wants users to be able to override data-* attributes (data-resin-target, data-target-id, data-testid) through navButtonProps for custom tabs. The spread of {...navButtonProps} should come after the data-* attributes to allow overriding, not before them.
Applied to files:
src/elements/content-sidebar/additional-tabs/AdditionalTabs.jssrc/elements/content-sidebar/additional-tabs/AdditionalTab.scsssrc/elements/content-sidebar/SidebarNav.jssrc/elements/content-sidebar/SidebarNavSignButton.tsxsrc/elements/content-sidebar/SidebarNavButton.jssrc/components/sidebar-toggle-button/SidebarToggleButton.jssrc/elements/content-sidebar/additional-tabs/AdditionalTabs.scsssrc/components/sidebar-toggle-button/SidebarToggleButton.scsssrc/elements/content-sidebar/SidebarNavSignButton.scsssrc/elements/content-sidebar/SidebarNavButton.scss
📚 Learning: 2025-09-03T18:30:44.447Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarPanels.js:0-0
Timestamp: 2025-09-03T18:30:44.447Z
Learning: In the CustomSidebarPanel type, the component field is required (React.ComponentType<any>), so runtime checks for component existence are unnecessary since Flow will catch missing components at compile time. User fpan225 prefers to rely on the type system rather than adding redundant runtime checks.
Applied to files:
src/elements/content-sidebar/additional-tabs/AdditionalTabs.jssrc/elements/content-sidebar/SidebarNavSignButton.tsxsrc/elements/content-sidebar/SidebarNavButton.js
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Applied to files:
src/elements/content-sidebar/SidebarNav.jssrc/elements/content-sidebar/SidebarNavSignButton.tsxsrc/elements/content-sidebar/SidebarNavButton.jssrc/components/sidebar-toggle-button/SidebarToggleButton.js
📚 Learning: 2025-09-03T18:10:29.467Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarNav.js:94-106
Timestamp: 2025-09-03T18:10:29.467Z
Learning: In the ContentSidebar CustomSidebarPanel API, the navButtonProps spread should come after explicit data-* attributes to allow users to override analytics tracking attributes like data-resin-target, data-target-id, and data-testid when needed. This is intentional API design for flexibility.
Applied to files:
src/elements/content-sidebar/SidebarNav.jssrc/elements/content-sidebar/SidebarNavSignButton.tsxsrc/elements/content-sidebar/SidebarNavButton.jssrc/elements/content-sidebar/SidebarNavButton.scss
📚 Learning: 2025-07-11T14:43:02.677Z
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
Learning: In the box-ui-elements repository, there's a file-type-based pattern for internationalization: TypeScript files (.tsx) predominantly use the modern useIntl hook (41 vs 15 files), while JavaScript files (.js) predominantly use the legacy injectIntl HOC (64 vs 5 files). New TypeScript components should use useIntl, while existing JavaScript components typically continue using injectIntl for consistency.
Applied to files:
src/elements/content-sidebar/SidebarNav.jssrc/components/sidebar-toggle-button/SidebarToggleButton.js
📚 Learning: 2025-06-25T13:09:54.538Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 4160
File: src/elements/content-sidebar/SidebarToggle.js:21-27
Timestamp: 2025-06-25T13:09:54.538Z
Learning: The box-ui-elements project uses Flow for type annotations in JavaScript files, as indicated by flow directives in file headers. Type annotations like `: Props` are valid Flow syntax, not TypeScript syntax.
Applied to files:
src/elements/content-sidebar/SidebarNav.js
📚 Learning: 2025-06-25T13:09:45.168Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 4160
File: src/elements/content-sidebar/SidebarToggle.js:13-19
Timestamp: 2025-06-25T13:09:45.168Z
Learning: Files with `flow` or `flow strict` comments at the top use Flow type syntax, not TypeScript. Flow type definitions like `type Props = { ... }` and type imports like `type { RouterHistory }` are valid Flow syntax and should not be flagged as TypeScript-only features.
Applied to files:
src/elements/content-sidebar/SidebarNav.js
📚 Learning: 2025-06-25T13:10:19.128Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 4160
File: src/elements/content-sidebar/SidebarToggle.js:11-11
Timestamp: 2025-06-25T13:10:19.128Z
Learning: In the box-ui-elements codebase, Flow files support `import type` syntax for type-only imports. The `import type` statement is valid Flow syntax, not just TypeScript, and should not be flagged as an error when used in Flow files (indicated by flow comments).
Applied to files:
src/elements/content-sidebar/SidebarNav.js
📚 Learning: 2025-09-09T21:43:34.180Z
Learnt from: bxie-box
Repo: box/box-ui-elements PR: 4271
File: src/features/classification/README.md:67-67
Timestamp: 2025-09-09T21:43:34.180Z
Learning: In the Classification component's aiClassificationReason prop, TooltipProvider is only required when the citations array is empty (shows tooltip UI), not when citations are present (shows card on hover UI).
Applied to files:
src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.jssrc/elements/content-sidebar/SidebarNavSignButton.tsx
📚 Learning: 2025-08-15T14:42:01.840Z
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4227
File: src/elements/common/sub-header/SubHeader.tsx:19-19
Timestamp: 2025-08-15T14:42:01.840Z
Learning: In SubHeader.tsx, the bulkItemActionMenuProps prop is intentionally required (not optional) because there will always be a default "Download" action available, ensuring the prop is never undefined in actual usage.
Applied to files:
src/elements/content-sidebar/SidebarNavButton.js
🧬 Code graph analysis (6)
src/elements/content-sidebar/additional-tabs/AdditionalTabs.js (3)
src/elements/content-sidebar/SidebarNav.js (1)
isPreviewModernizationEnabled(96-96)src/components/sidebar-toggle-button/SidebarToggleButton.js (1)
isPreviewModernizationEnabled(35-35)src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js (1)
isPreviewModernizationEnabled(23-23)
src/elements/content-sidebar/SidebarNav.js (7)
src/components/sidebar-toggle-button/SidebarToggleButton.js (1)
isPreviewModernizationEnabled(35-35)src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js (1)
isPreviewModernizationEnabled(23-23)src/elements/common/feature-checking/hooks.js (1)
useFeatureConfig(6-9)src/elements/content-sidebar/messages.js (1)
messages(9-115)src/elements/content-sidebar/SidebarNavButton.js (1)
SidebarNavButton(37-37)src/elements/content-sidebar/Sidebar.js (3)
hasActivity(320-320)hasDetails(321-321)hasSkills(323-323)src/elements/common/interactionTargets.js (2)
SIDEBAR_NAV_TARGETS(2-11)SIDEBAR_NAV_TARGETS(2-11)
src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js (1)
src/elements/common/feature-checking/hooks.js (1)
useFeatureConfig(6-9)
src/elements/content-sidebar/SidebarNavSignButton.tsx (2)
src/elements/common/feature-checking/hooks.js (1)
useFeatureConfig(6-9)src/elements/content-sidebar/messages.js (1)
messages(9-115)
src/elements/content-sidebar/SidebarNavButton.js (1)
src/utils/dom.js (1)
isLeftClick(79-81)
src/components/sidebar-toggle-button/SidebarToggleButton.js (3)
src/elements/content-sidebar/SidebarNav.js (1)
isPreviewModernizationEnabled(96-96)src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js (1)
isPreviewModernizationEnabled(23-23)src/elements/common/feature-checking/hooks.js (1)
useFeatureConfig(6-9)
🪛 Biome (2.1.2)
src/elements/content-sidebar/SidebarNav.js
[error] 110-110: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 121-121: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 132-132: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/SidebarNavButton.js
[error] 14-18: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ 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). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (19)
src/elements/content-sidebar/SidebarNavSignButton.scss (1)
17-23: LGTM!The modernized SVG scaling implementation is consistent with the broader modernization pattern across the sidebar components.
src/elements/content-sidebar/additional-tabs/AdditionalTab.scss (2)
15-23: LGTM!The modernized styling correctly handles both SVG and IMG elements for additional tab icons, ensuring consistent behavior regardless of icon type.
26-26: LGTM!The explicit duration format (0.5s) improves readability and consistency.
src/elements/content-sidebar/_mixins.scss (2)
17-17: LGTM!The explicit opacity format (0.5) improves readability and aligns with modern CSS conventions.
30-33: LGTM!The new mixin provides standardized 20px sizing for modernized icons, creating visual consistency across the modernized sidebar navigation.
src/components/sidebar-toggle-button/SidebarToggleButton.scss (1)
33-45: LGTM!The modernized styling correctly handles both legacy (
.is-disabled) and current (.bdl-is-disabled) class patterns, ensuring backward compatibility while providing consistent SVG scaling across all toggle button states.src/elements/content-sidebar/additional-tabs/AdditionalTabs.scss (2)
1-2: LGTM!The import is necessary for the
bdl-SidebarNavIconModernizedmixin used in the modernized modifier block.
7-12: LGTM!The modernized modifier correctly applies the standardized 20px icon sizing to both regular and placeholder tab icons when modernization is enabled.
src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js (3)
8-8: LGTM!The imports are necessary for the Blueprint tooltip integration and feature flag checking.
Also applies to: 11-11
23-23: LGTM!The feature flag check follows the established pattern used across other modernized components.
26-33: The span wrapper is architecturally justified for this wrapper component.AdditionalTabTooltip is a generic wrapper component that accepts children as a prop, not the owner/controller of the button. The span wrapper is necessary because BPTooltip requires a DOM element to attach to, and this wrapper cannot assume its children (which come from consumers) will forward refs. This pattern is consistent with similar modernization code elsewhere in the codebase (e.g., SidebarToggleButton.js). The suggestion to update the button component to forward refs should be addressed at the call sites where buttons are passed as children, not here.
Likely an incorrect or invalid review comment.
src/elements/content-sidebar/additional-tabs/AdditionalTabs.js (3)
8-8: LGTM!The classNames utility is appropriate for conditional CSS class application.
17-17: LGTM!The optional boolean prop is correctly typed for the feature flag.
58-66: LGTM!The conditional class application correctly enables modernized styling through CSS cascade, without needing to pass the prop to child components.
src/elements/content-sidebar/SidebarNavButton.scss (2)
27-27: LGTM!The single-quote format normalizes the attribute selector style and maintains consistency with the new modernized block.
48-57: LGTM!The modernized styling correctly applies SVG scaling to hover, focus, and selected states using semantic ARIA attributes, consistent with the broader modernization pattern.
src/components/sidebar-toggle-button/SidebarToggleButton.js (1)
6-6: BPTooltip integration and feature-flag path look solidThe previewModernization branch correctly introduces BPTooltip with an opposite-side tooltip, applies the modernized CSS modifier class, and falls back cleanly to the legacy Tooltip path when the flag is disabled. No issues spotted with the hook usage or button rendering.
Also applies to: 11-11, 35-42, 51-64
src/elements/content-sidebar/SidebarNav.js (2)
11-21: Icon wrappers and Flow annotations align with modernization goalsCentralizing icon sizing in
SIDEBAR_TAB_ICON_PROPSand using the filled vs outline Blueprint icons in theActivityIconWrapper,DetailsIconWrapper, andMetadataIconWrapperbased onisPreviewModernizationEnabledandisActivegives clear, consistent visual states. The inline parameter type annotations (({ isActive }: { isActive?: boolean })) are valid Flow syntax in this codebase; the Biome “TypeScript-only” parse errors for these lines are false positives and should be handled via linter configuration rather than code changes. Based on learningsAlso applies to: 47-50, 96-142
143-149: Modernized nav container, buttons, and overflow wiring look consistentApplying
bcs-SidebarNav--modernizedandbcs-SidebarNav-overflow--modernizedbased onisPreviewModernizationEnabled, while threading the same flag into eachSidebarNavButtonandAdditionalTabs, keeps the legacy behavior intact and cleanly scopes modern styles behind the feature flag. Data attributes and existing tab wiring remain unchanged, so analytics and routing behavior should preserve current behavior.Also applies to: 159-237, 255-266
| <span> | ||
| <PlainButton aria-label={intlText} className={classes} onClick={onClick} type="button" {...rest}> | ||
| {renderButton()} | ||
| </PlainButton> | ||
| </span> |
There was a problem hiding this comment.
why not use Blueprint button?
| width: 20px; | ||
| height: 20px; |
| display: flex; | ||
| flex-direction: column; | ||
|
|
||
| &--modernized { |
There was a problem hiding this comment.
in the past, we tried to write out the entire classname for searchability:
.bdl-AdditionalTabs--modernized
2 participants
Summary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.