Skip to content

Conversation

@loonskai
Copy link
Contributor

@loonskai loonskai commented Nov 20, 2025

Sidebar toggle button now supports custom rendering, enabling developers to tailor its appearance and interactions.

Usage example

import ContentPreview from 'box-ui-elements/es/elements/content-preview';

<ContentPreview
  {...rest}
  renderToggleButton: ({ isOpen, ...toggleButtonProps }: { isOpen: boolean }) => {
      return <SidebarToggleButton sidebarToggleState={isOpen} {...toggleButtonProps} />;
  }
 />

2025-11-20 14 18 27

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 20, 2025

Walkthrough

Added SidebarToggleButtonProps type definition and threaded renderToggleButton prop through the sidebar component hierarchy (ContentSidebar → Sidebar → SidebarNav → SidebarToggle) to enable custom toggle button rendering via the render prop pattern, with supporting tests added.

Changes

Cohort / File(s) Summary
Type Definitions
src/elements/common/types/SidebarNavigation.js.flow, src/elements/common/types/SidebarNavigation.ts
Added new public type SidebarToggleButtonProps with optional data-resin-target and data-testid strings, optional isOpen boolean, and required onClick handler accepting SyntheticMouseEvent<HTMLButtonElement> / React.MouseEvent<HTMLButtonElement>
Component Props Threading
src/elements/content-sidebar/ContentSidebar.js, src/elements/content-sidebar/Sidebar.js, src/elements/content-sidebar/SidebarNav.js
Added optional renderToggleButton prop to each component's public API; prop forwarded through component hierarchy to enable custom toggle rendering
Toggle Button Logic
src/elements/content-sidebar/SidebarToggle.js
Imported SidebarToggleButtonProps type; added renderToggleButton prop; refactored click handler to use React.useCallback with explicit event typing; created memoized renderProps object containing isOpen, onClick, and data attributes; conditionally render custom toggle or default SidebarToggleButton
Test Coverage
src/elements/content-sidebar/__tests__/SidebarToggle.test.js
Added test suite for custom render path verifying custom toggle renders, receives correct props (isOpen, onClick, data attributes), and history state updates work correctly

Sequence Diagram

sequenceDiagram
    participant App as ContentSidebar
    participant Sidebar
    participant SidebarNav
    participant SidebarToggle as SidebarToggle
    participant CustomRenderer as Custom Renderer<br/>(if provided)
    participant DefaultButton as SidebarToggleButton<br/>(default)

    App->>Sidebar: renderToggleButton prop
    Sidebar->>SidebarNav: renderToggleButton prop
    SidebarNav->>SidebarToggle: renderToggleButton prop
    
    Note over SidebarToggle: On mount/re-render
    SidebarToggle->>SidebarToggle: Create renderProps<br/>{isOpen, onClick, data-*}
    
    alt renderToggleButton provided
        SidebarToggle->>CustomRenderer: renderToggleButton(renderProps)
        CustomRenderer-->>SidebarToggle: Custom JSX
    else renderToggleButton not provided
        SidebarToggle->>DefaultButton: Render with<br/>onClick handler
        DefaultButton-->>SidebarToggle: Default JSX
    end
    
    Note over SidebarToggle: On click
    SidebarToggle->>SidebarToggle: handleToggleClick invoked
    SidebarToggle->>SidebarToggle: Update state via<br/>history.replace or<br/>internalSidebarNavigationHandler
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

  • SidebarToggle.js — Requires careful review of React.useCallback logic, renderProps memoization, and conditional rendering path; ensure event handling consistency with router-enabled/disabled flows
  • Prop threading consistency — Verify renderToggleButton correctly forwarded through all four components (ContentSidebar → Sidebar → SidebarNav → SidebarToggle)
  • Type safety — Confirm SidebarToggleButtonProps type completeness and proper usage in both Flow and TypeScript versions

Possibly related PRs

Suggested labels

ready-to-merge

Suggested reviewers

  • jankowiakdawid
  • greg-in-a-box
  • tjiang-box

Poem

🐰 A toggle button's freedom rings,
Custom renders, oh what joy it brings!
Through props we weave, from top to ground,
Sidebar dance, so smooth and sound!
One click, one render—choice profound! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is empty except for boilerplate template comments about merge procedures and contains no actual description of the changes being made. Add a meaningful description explaining the motivation, implementation approach, and key changes introduced by this PR beyond the template boilerplate.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding a renderToggleButton prop to enable custom toggle component rendering in the sidebar.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between abc11ee and b6599b4.

📒 Files selected for processing (7)
  • src/elements/common/types/SidebarNavigation.js.flow (1 hunks)
  • src/elements/common/types/SidebarNavigation.ts (1 hunks)
  • src/elements/content-sidebar/ContentSidebar.js (4 hunks)
  • src/elements/content-sidebar/Sidebar.js (4 hunks)
  • src/elements/content-sidebar/SidebarNav.js (4 hunks)
  • src/elements/content-sidebar/SidebarToggle.js (1 hunks)
  • src/elements/content-sidebar/__tests__/SidebarToggle.test.js (4 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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.
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.
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.
📚 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/common/types/SidebarNavigation.ts
  • src/elements/common/types/SidebarNavigation.js.flow
  • src/elements/content-sidebar/SidebarToggle.js
  • src/elements/content-sidebar/__tests__/SidebarToggle.test.js
  • src/elements/content-sidebar/Sidebar.js
  • src/elements/content-sidebar/ContentSidebar.js
  • src/elements/content-sidebar/SidebarNav.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/common/types/SidebarNavigation.ts
  • src/elements/common/types/SidebarNavigation.js.flow
  • src/elements/content-sidebar/SidebarToggle.js
  • src/elements/content-sidebar/__tests__/SidebarToggle.test.js
  • src/elements/content-sidebar/Sidebar.js
  • src/elements/content-sidebar/ContentSidebar.js
  • src/elements/content-sidebar/SidebarNav.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/common/types/SidebarNavigation.ts
  • src/elements/common/types/SidebarNavigation.js.flow
  • src/elements/content-sidebar/SidebarToggle.js
  • src/elements/content-sidebar/__tests__/SidebarToggle.test.js
  • src/elements/content-sidebar/Sidebar.js
  • src/elements/content-sidebar/ContentSidebar.js
  • src/elements/content-sidebar/SidebarNav.js
📚 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/common/types/SidebarNavigation.js.flow
  • src/elements/content-sidebar/SidebarToggle.js
  • src/elements/content-sidebar/Sidebar.js
  • src/elements/content-sidebar/ContentSidebar.js
  • src/elements/content-sidebar/SidebarNav.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/common/types/SidebarNavigation.js.flow
  • src/elements/content-sidebar/Sidebar.js
  • src/elements/content-sidebar/ContentSidebar.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/common/types/SidebarNavigation.js.flow
  • src/elements/content-sidebar/Sidebar.js
  • src/elements/content-sidebar/ContentSidebar.js
  • src/elements/content-sidebar/SidebarNav.js
📚 Learning: 2025-08-27T17:03:48.322Z
Learnt from: bxie-box
Repo: box/box-ui-elements PR: 4250
File: src/features/classification/applied-by-ai-classification-reason/__tests__/AppliedByAiClassificationReason.test.tsx:44-51
Timestamp: 2025-08-27T17:03:48.322Z
Learning: In test files for bxie-box, prefer using queryByTestId over getByTestId when testing for expected elements that should always exist, as null access serves as validation for regressions or unexpected changes rather than masking issues with defensive assertions.

Applied to files:

  • src/elements/content-sidebar/__tests__/SidebarToggle.test.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/Sidebar.js
  • src/elements/content-sidebar/ContentSidebar.js
📚 Learning: 2025-06-11T16:30:10.431Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 4136
File: src/elements/common/types/SidebarNavigation.ts:16-26
Timestamp: 2025-06-11T16:30:10.431Z
Learning: `VersionSidebarView` intentionally uses the `versionId` field to stay consistent with current URL parameter naming; a potential rename to `fileVersionId` is deferred until React Router is removed.

Applied to files:

  • src/elements/content-sidebar/Sidebar.js
  • src/elements/content-sidebar/ContentSidebar.js
🧬 Code graph analysis (5)
src/elements/content-sidebar/SidebarToggle.js (4)
src/elements/content-sidebar/ContentSidebar.js (4)
  • Props (227-227)
  • Props (276-276)
  • Props (321-321)
  • Props (353-386)
src/elements/content-sidebar/Sidebar.js (2)
  • Props (295-320)
  • isOpen (321-321)
src/elements/common/types/SidebarNavigation.ts (3)
  • InternalSidebarNavigation (49-51)
  • InternalSidebarNavigationHandler (55-55)
  • SidebarToggleButtonProps (57-62)
src/elements/content-sidebar/__tests__/SidebarToggle.test.js (1)
  • renderToggleButton (93-97)
src/elements/content-sidebar/__tests__/SidebarToggle.test.js (2)
src/features/collapsible-sidebar/CollapsibleSidebarLogo.js (1)
  • toggleButton (89-99)
src/elements/content-sidebar/SidebarToggle.js (1)
  • SidebarToggle (27-78)
src/elements/content-sidebar/Sidebar.js (2)
src/elements/common/types/SidebarNavigation.ts (1)
  • SidebarToggleButtonProps (57-62)
src/elements/content-sidebar/__tests__/SidebarToggle.test.js (1)
  • renderToggleButton (93-97)
src/elements/content-sidebar/ContentSidebar.js (2)
src/elements/common/types/SidebarNavigation.ts (1)
  • SidebarToggleButtonProps (57-62)
src/elements/content-sidebar/__tests__/SidebarToggle.test.js (1)
  • renderToggleButton (93-97)
src/elements/content-sidebar/SidebarNav.js (2)
src/elements/common/types/SidebarNavigation.ts (1)
  • SidebarToggleButtonProps (57-62)
src/elements/content-sidebar/__tests__/SidebarToggle.test.js (1)
  • renderToggleButton (93-97)
🪛 Biome (2.1.2)
src/elements/content-sidebar/SidebarToggle.js

[error] 12-16: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 17-25: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 34-34: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 36-36: 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/Sidebar.js

[error] 34-34: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)

src/elements/content-sidebar/ContentSidebar.js

[error] 51-51: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)

src/elements/content-sidebar/SidebarNav.js

[error] 38-42: '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 (8)
src/elements/common/types/SidebarNavigation.ts (1)

57-62: Well-designed type definition for the render prop pattern.

The SidebarToggleButtonProps type provides a clean API surface for custom toggle button implementations. The structure appropriately makes data attributes and isOpen optional while requiring onClick, giving consumers flexibility while ensuring core functionality.

src/elements/content-sidebar/__tests__/SidebarToggle.test.js (1)

91-133: Excellent test coverage for the custom render path.

The test suite comprehensively validates the renderToggleButton functionality:

  • Confirms custom component renders in the DOM
  • Verifies all required props are passed correctly (isOpen, onClick, data-resin-target, data-testid)
  • Validates click interaction triggers the expected history state change

This thorough coverage will catch regressions in the custom render API.

src/elements/content-sidebar/ContentSidebar.js (1)

51-51: Clean integration of the custom toggle button API.

The renderToggleButton prop is properly typed, imported, and threaded through to the Sidebar component, exposing this customization capability at the public ContentSidebar API level.

Also applies to: 94-94, 382-382, 426-426

src/elements/content-sidebar/Sidebar.js (1)

34-34: Proper prop forwarding through the component hierarchy.

The Sidebar component correctly receives and forwards renderToggleButton to SidebarNav, maintaining the clean data flow from the public API down to the toggle implementation.

Also applies to: 74-74, 316-316, 357-357

src/elements/common/types/SidebarNavigation.js.flow (1)

43-48: Type definition consistent with TypeScript counterpart.

The Flow type definition mirrors the TypeScript version with appropriate Flow syntax (SyntheticMouseEvent<HTMLButtonElement>), ensuring type safety across both type systems used in the codebase.

src/elements/content-sidebar/SidebarNav.js (1)

38-42: Seamless integration in the navigation component.

The SidebarNav component correctly imports the type, accepts the prop, and forwards it to SidebarToggle, completing the prop threading from the public API to the final implementation.

Also applies to: 65-65, 85-85, 212-212

src/elements/content-sidebar/SidebarToggle.js (2)

35-55: Well-implemented event handler with proper memoization.

The handleToggleClick callback correctly:

  • Uses useCallback with complete dependencies
  • Handles both router-enabled and router-disabled modes
  • Prevents default behavior appropriately
  • Passes the replace flag for toggle actions

57-77: Clean render prop implementation following React best practices.

The implementation properly:

  • Memoizes renderProps to prevent unnecessary re-renders
  • Provides all necessary props (isOpen, onClick, data attributes) to custom renderers
  • Falls back to the default SidebarToggleButton when no custom renderer is provided
  • Maintains consistent data attributes between custom and default render paths

This gives consumers full control over toggle rendering while ensuring core functionality is preserved.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@loonskai loonskai force-pushed the sidebar-toggle-custom-render branch from 96c30a5 to eba34e5 Compare November 20, 2025 11:58
@loonskai loonskai self-assigned this Nov 20, 2025
@loonskai loonskai marked this pull request as ready for review November 20, 2025 13:07
@loonskai loonskai requested review from a team as code owners November 20, 2025 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants