refactor(preview-sidebar): add modernized version of sidebar toggle button #4390
Conversation
|
|
WalkthroughAdds a feature-flagged modernized render path to SidebarToggleButton with breakpoint-aware IconButton icons and tooltip positioning, updates tests for both render paths, pins Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-09-03T18:10:42.760ZApplied to files:
📚 Learning: 2025-09-03T18:10:29.467ZApplied to files:
📚 Learning: 2025-06-11T16:30:10.431ZApplied to files:
📚 Learning: 2025-07-11T14:43:02.677ZApplied to files:
🧬 Code graph analysis (1)src/components/sidebar-toggle-button/SidebarToggleButton.js (3)
⏰ 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). (3)
🔇 Additional comments (8)
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
🧹 Nitpick comments (3)
src/icons/general/IconRightSidebarChevronOpen.js.flow (1)
6-6: Useimport typefor Flow type imports.For clarity and to ensure the import is erased at compile time, use
import typesyntax for type-only imports in Flow.-import { Icon } from '../iconTypes'; +import type { Icon } from '../iconTypes';src/icons/general/IconRightSidebarChevronClose.js.flow (1)
6-6: Useimport typefor Flow type imports.-import { Icon } from '../iconTypes'; +import type { Icon } from '../iconTypes';src/components/sidebar-toggle-button/SidebarToggleButton.js (1)
57-75: Reduce duplication in IconButton rendering.The two
IconButtonblocks are nearly identical, differing only in the icon selection logic. Consider extracting the icon selection into a variable to eliminate duplication.if (isPreviewModernizationEnabled) { const tooltipPositionModernized = isDirectionLeft ? DIRECTION_RIGHT : DIRECTION_LEFT; + const modernizedIcon = isDirectionLeft + ? (isOpen ? IconRightSidebarChevronOpen : IconRightSidebarChevronClose) + : (isOpen ? IconRightSidebarChevronClose : IconRightSidebarChevronOpen); return ( <BPTooltip content={intlText} side={tooltipPositionModernized}> - {isDirectionLeft ? ( - <IconButton - aria-label={intlText} - icon={isOpen ? IconRightSidebarChevronOpen : IconRightSidebarChevronClose} - onClick={onClick} - size="large" - variant="high-contrast" - {...rest} - /> - ) : ( - <IconButton - aria-label={intlText} - icon={isOpen ? IconRightSidebarChevronClose : IconRightSidebarChevronOpen} - onClick={onClick} - size="large" - variant="high-contrast" - {...rest} - /> - )} + <IconButton + aria-label={intlText} + icon={modernizedIcon} + onClick={onClick} + size="large" + variant="high-contrast" + {...rest} + /> </BPTooltip> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/sidebar-toggle-button/SidebarToggleButton.js(2 hunks)src/icons/general/IconRightSidebarChevronClose.js.flow(1 hunks)src/icons/general/IconRightSidebarChevronClose.tsx(1 hunks)src/icons/general/IconRightSidebarChevronOpen.js.flow(1 hunks)src/icons/general/IconRightSidebarChevronOpen.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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/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.
📚 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/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/components/sidebar-toggle-button/SidebarToggleButton.js
📚 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/components/sidebar-toggle-button/SidebarToggleButton.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/icons/general/IconRightSidebarChevronOpen.js.flowsrc/icons/general/IconRightSidebarChevronClose.js.flow
📚 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/icons/general/IconRightSidebarChevronOpen.js.flowsrc/icons/general/IconRightSidebarChevronClose.js.flow
📚 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/icons/general/IconRightSidebarChevronOpen.js.flowsrc/icons/general/IconRightSidebarChevronClose.js.flow
📚 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/icons/general/IconRightSidebarChevronOpen.js.flowsrc/icons/general/IconRightSidebarChevronClose.js.flow
🧬 Code graph analysis (1)
src/components/sidebar-toggle-button/SidebarToggleButton.js (1)
src/elements/content-sidebar/SidebarNav.js (1)
isPreviewModernizationEnabled(139-139)
⏰ 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 (2)
src/components/sidebar-toggle-button/SidebarToggleButton.js (2)
46-51: Approve non-modernized icon rendering logic.The
renderIconfunction cleanly handles the direction-based icon selection for the legacy path, with proper sizing.
53-56: Modernized rendering path looks good.The feature flag check and tooltip positioning logic are correctly implemented, consistent with the pattern used in other components like
SidebarNav.jsandAdditionalTabTooltip.js.
jmcbgaston
force-pushed
the
modernize-sidebar-toggle-button
branch
from
7f603b1 to
a2de97c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/icons/general/IconRightSidebarChevronClose.tsx (1)
10-22: SVG attributes correctly updated to camelCase.The previously flagged kebab-case attributes have been fixed. All SVG attributes now use proper JSX camelCase (
fillOpacity,fillRule,clipRule).src/icons/general/IconRightSidebarChevronClose.js.flow (1)
11-23: SVG attributes correctly updated to camelCase.The Flow version also has the corrected camelCase attributes. Implementation is consistent with the TypeScript version.
src/icons/general/IconRightSidebarChevronOpen.tsx (1)
10-22: SVG attributes correctly updated to camelCase.All JSX attributes are properly formatted in camelCase. The implementation is consistent with the companion Close icon.
🧹 Nitpick comments (1)
src/components/sidebar-toggle-button/SidebarToggleButton.js (1)
64-84: Consider reducing duplication in IconButton blocks.The left and right direction branches (lines 64-73 and 74-83) are nearly identical, differing only in the icon prop. Consider extracting the icon selection logic into a variable to eliminate duplication.
Apply this refactor:
if (isPreviewModernizationEnabled) { const tooltipPositionModernized = isDirectionLeft ? DIRECTION_RIGHT : DIRECTION_LEFT; + const selectedIcon = isDirectionLeft + ? (isOpen ? IconRightSidebarChevronOpen : IconRightSidebarChevronClose) + : (isOpen ? IconRightSidebarChevronClose : IconRightSidebarChevronOpen); return ( <BPTooltip content={intlText} side={tooltipPositionModernized}> - {isDirectionLeft ? ( - <IconButton - aria-label={intlText} - icon={isOpen ? IconRightSidebarChevronOpen : IconRightSidebarChevronClose} - onClick={onClick} - onMouseDown={mouseDownHandler} - size="large" - variant="high-contrast" - {...rest} - /> - ) : ( - <IconButton - aria-label={intlText} - icon={isOpen ? IconRightSidebarChevronClose : IconRightSidebarChevronOpen} - onClick={onClick} - onMouseDown={mouseDownHandler} - size="large" - variant="high-contrast" - {...rest} - /> - )} + <IconButton + aria-label={intlText} + icon={selectedIcon} + onClick={onClick} + onMouseDown={mouseDownHandler} + size="large" + variant="high-contrast" + {...rest} + /> </BPTooltip> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/sidebar-toggle-button/SidebarToggleButton.js(4 hunks)src/icons/general/IconRightSidebarChevronClose.js.flow(1 hunks)src/icons/general/IconRightSidebarChevronClose.tsx(1 hunks)src/icons/general/IconRightSidebarChevronOpen.js.flow(1 hunks)src/icons/general/IconRightSidebarChevronOpen.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/icons/general/IconRightSidebarChevronOpen.js.flow
🧰 Additional context used
🧠 Learnings (8)
📓 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.
📚 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/icons/general/IconRightSidebarChevronClose.js.flowsrc/icons/general/IconRightSidebarChevronOpen.tsx
📚 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/icons/general/IconRightSidebarChevronClose.js.flow
📚 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/icons/general/IconRightSidebarChevronClose.js.flowsrc/icons/general/IconRightSidebarChevronOpen.tsx
📚 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/icons/general/IconRightSidebarChevronClose.js.flow
📚 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/icons/general/IconRightSidebarChevronClose.js.flowsrc/icons/general/IconRightSidebarChevronClose.tsxsrc/components/sidebar-toggle-button/SidebarToggleButton.jssrc/icons/general/IconRightSidebarChevronOpen.tsx
📚 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/components/sidebar-toggle-button/SidebarToggleButton.js
📚 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/components/sidebar-toggle-button/SidebarToggleButton.js
⏰ 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 (3)
src/components/sidebar-toggle-button/SidebarToggleButton.js (3)
6-6: Appropriate imports for modernization.The blueprint-web components (
IconButton,BPTooltip) and new chevron icons are correctly imported to support the modernized rendering path.Also applies to: 9-10
44-51: Clean refactoring of helper logic.Extracting
isDirectionLeftand using it for tooltip positioning improves readability. TherenderIconfunction appropriately maintains the existing icon logic for the non-modernized path.
60-101: Modernization approach maintains backward compatibility.The feature-flagged modernization path correctly uses blueprint-web components while preserving the existing non-modernized behavior. The icon selection logic is correct for both left and right direction sidebars, and the
mouseDownevent handling is consistently applied in both paths.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/sidebar-toggle-button/__tests__/SidebarToggleButton.test.js (1)
60-70: Stable test hook viadata-testidlooks good; consider aligning name and helperSwitching to
screen.getByTestId('sidebar-toggle-button')matches the new shareddata-testidon both legacy and modernized paths, so the click assertion remains robust. You may want to (a) tweak the test description to focus on click behavior now that it no longer inspects the rendered variant, and/or (b) switch toqueryByTestIdplus an explicit existence assertion to match our preferred pattern for test IDs. Based on learnings, this follows the existing guidance aroundqueryByTestIdusage.src/components/sidebar-toggle-button/SidebarToggleButton.js (1)
6-10: Modernized IconButton path and chevron icons look consistent; consider mirroringtype="button"Using
BPTooltipplusIconButtonfor the modernization branch, with direction-aware chevron icons and a shareddata-testid="sidebar-toggle-button", keeps behavior aligned with the legacy path while preserving theonMouseDownstop-propagation behavior covered by tests. To fully mirror the non-modernizedPlainButtonusage and avoid any risk of accidental form submission, consider also passingtype="button"through toIconButton, assuming its API forwards that prop to the underlying button element; worth a quick check against the@box/blueprint-webIconButton contract.Also applies to: 61-86
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/sidebar-toggle-button/SidebarToggleButton.js(4 hunks)src/components/sidebar-toggle-button/__tests__/SidebarToggleButton.test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 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.
📚 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/components/sidebar-toggle-button/__tests__/SidebarToggleButton.test.js
📚 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/components/sidebar-toggle-button/__tests__/SidebarToggleButton.test.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/components/sidebar-toggle-button/__tests__/SidebarToggleButton.test.jssrc/components/sidebar-toggle-button/SidebarToggleButton.js
📚 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/components/sidebar-toggle-button/SidebarToggleButton.js
⏰ 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 (1)
src/components/sidebar-toggle-button/SidebarToggleButton.js (1)
44-47: Directional helper +renderIconreuse keep behavior consistent and support testingExtracting
isDirectionLeft,tooltipPosition, andrenderIconkeeps the existing left/right + open/closed icon logic intact while simplifying reuse between branches. Addingdata-testid="sidebar-toggle-button"to thePlainButtonwith{...rest}spread last gives tests a stable hook but still lets consumers override attributes (includingdata-testid) when needed, which matches the intended override pattern for data-* props. Based on learnings, this ordering is aligned with prior SidebarNav design.Also applies to: 91-102
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
package.json(2 hunks)src/components/sidebar-toggle-button/SidebarToggleButton.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/sidebar-toggle-button/SidebarToggleButton.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 4224
File: package.json:296-297
Timestamp: 2025-08-12T18:04:17.698Z
Learning: In the box-ui-elements project, the team is comfortable with raising peerDependency minimum versions when upgrading blueprint-web packages, even if it's a breaking change for consumers.
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/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.
📚 Learning: 2025-08-12T18:04:17.698Z
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 4224
File: package.json:296-297
Timestamp: 2025-08-12T18:04:17.698Z
Learning: In the box-ui-elements project, the team is comfortable with raising peerDependency minimum versions when upgrading blueprint-web packages, even if it's a breaking change for consumers.
Applied to files:
package.json
⏰ 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 (1)
package.json (1)
128-128: Consistent dependency and peerDependency versions.Both
devDependenciesandpeerDependenciesare updated in sync, which is good practice. Based on learnings, the team is comfortable with raising blueprint-web peerDependency versions.Also applies to: 300-300
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/sidebar-toggle-button/SidebarToggleButton.scss (1)
40-47: Consider moving transition outside hover for symmetrical animation.The transition property is currently inside the
:hoverstate, which means it applies when hovering in but not when hovering out. For a smoother user experience, move the transition to the base element.Apply this diff to enable symmetrical hover animation:
.bcs-SidebarNav--modernized { .bdl-SidebarToggleButton--modernized { + svg { + transition: transform 150ms; + } + &:hover svg { transform: scale(1.09); - transition: transform 150ms; } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/sidebar-toggle-button/SidebarToggleButton.js(3 hunks)src/components/sidebar-toggle-button/SidebarToggleButton.scss(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/sidebar-toggle-button/SidebarToggleButton.js
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 4224
File: package.json:296-297
Timestamp: 2025-08-12T18:04:17.698Z
Learning: In the box-ui-elements project, the team is comfortable with raising peerDependency minimum versions when upgrading blueprint-web packages, even if it's a breaking change for consumers.
📚 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/components/sidebar-toggle-button/SidebarToggleButton.scss
📚 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/components/sidebar-toggle-button/SidebarToggleButton.scss
⏰ 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). (3)
- GitHub Check: lint_test_build
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Summary
🔇 Additional comments (1)
src/components/sidebar-toggle-button/SidebarToggleButton.scss (1)
33-38: LGTM! Clean separation of modernized variant.The new
.bdl-SidebarToggleButton--modernizedclass properly establishes a stacking context to address the clickability issue. The explanatory comment is helpful and references the related file.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
300-300: Consider keeping the caret for peerDependency flexibility.The caret was removed, making this an exact version constraint (
4.88.2instead of^4.88.2). This forces consumers to install exactly version 4.88.2 and prevents them from using compatible newer versions (e.g., 4.88.3 or 4.89.0).If the exact version pin is intentional due to specific asset requirements for the modernized sidebar toggle, please confirm. Otherwise, consider restoring the caret to allow compatible updates.
Based on learnings, the team is comfortable with breaking changes for blueprint packages.
Apply this diff if caret should be restored:
- "@box/blueprint-web-assets": "4.88.2", + "@box/blueprint-web-assets": "^4.88.2",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
package.json(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 4224
File: package.json:296-297
Timestamp: 2025-08-12T18:04:17.698Z
Learning: In the box-ui-elements project, the team is comfortable with raising peerDependency minimum versions when upgrading blueprint-web packages, even if it's a breaking change for consumers.
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: 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.
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.
📚 Learning: 2025-08-12T18:04:17.698Z
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 4224
File: package.json:296-297
Timestamp: 2025-08-12T18:04:17.698Z
Learning: In the box-ui-elements project, the team is comfortable with raising peerDependency minimum versions when upgrading blueprint-web packages, even if it's a breaking change for consumers.
Applied to files:
package.json
⏰ 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). (3)
- GitHub Check: lint_test_build
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Summary
🔇 Additional comments (1)
package.json (1)
128-128: No action needed. Version 4.88.2 of @box/blueprint-web-assets is published and available on npm.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/sidebar-toggle-button/SidebarToggleButton.js (1)
65-94: LGTM! Well-structured modernized render path with breakpoint-aware icon selection.The implementation correctly handles three scenarios: mobile view with chevrons, desktop left-sidebar, and desktop right-sidebar. All necessary props, handlers, and accessibility attributes are properly maintained.
Consider adding a brief comment above
renderModernizedIconexplaining the icon naming convention (e.g., "RightSidebarChevronOpen represents the sidebar's visual state, not the action"), as the relationship between icon names and theisOpenprop might not be immediately intuitive for future maintainers.+ // Icon selection based on breakpoint and sidebar position + // Note: Icon names reflect the visual chevron direction, not the action const renderModernizedIcon = () => {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/sidebar-toggle-button/SidebarToggleButton.js(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 4224
File: package.json:296-297
Timestamp: 2025-08-12T18:04:17.698Z
Learning: In the box-ui-elements project, the team is comfortable with raising peerDependency minimum versions when upgrading blueprint-web packages, even if it's a breaking change for consumers.
📚 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/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/components/sidebar-toggle-button/SidebarToggleButton.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/components/sidebar-toggle-button/SidebarToggleButton.js
📚 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/components/sidebar-toggle-button/SidebarToggleButton.js
🧬 Code graph analysis (1)
src/components/sidebar-toggle-button/SidebarToggleButton.js (3)
src/elements/content-sidebar/SidebarNav.js (1)
isPreviewModernizationEnabled(139-139)src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js (1)
isPreviewModernizationEnabled(23-23)src/elements/common/feature-checking/hooks.js (1)
useFeatureConfig(6-9)
⏰ 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). (3)
- GitHub Check: lint_test_build
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Summary
🔇 Additional comments (6)
src/components/sidebar-toggle-button/SidebarToggleButton.js (6)
7-13: LGTM! Clean import additions for the modernized path.The Blueprint Web imports are properly structured, and the
BPTooltipalias appropriately avoids conflicts with the existing legacy Tooltip component.
43-44: LGTM! Proper hook usage for responsive and feature-flagged behavior.The
useBreakpointanduseFeatureConfighooks are correctly placed and follow the established patterns in the codebase.
49-53: LGTM! Proper conditional class application.The classNames logic correctly applies mutually exclusive classes based on the feature flag while maintaining the shared
bdl-is-collapsedstate class.
55-56: LGTM! Clear helper variables for direction logic.The
isDirectionLeftboolean and derivedtooltipPositionimprove readability and are correctly used in both render paths.
96-101: LGTM! Clean icon helper for the legacy render path.The
renderIconfunction provides clear, direction-aware icon selection using semantic icon names.
103-116: LGTM! Legacy render path properly maintained.The legacy path is well-preserved with a nice refactor that extracts icon rendering into the
renderIconhelper, improving code organization while maintaining all existing functionality.
jmcbgaston
force-pushed
the
modernize-sidebar-toggle-button
branch
from
48cf8d5 to
87fbef7
Compare
jmcbgaston
force-pushed
the
modernize-sidebar-toggle-button
branch
from
87fbef7 to
9590491
Compare
Merge Queue Status✅ The pull request has been merged at bde9f79 This pull request spent 6 seconds in the queue, with no time running CI. Required conditions to merge
|
5 participants
Screenshots
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.