feat(preview-sidebar): add modernized version of sidebar toggle button #4389
Conversation
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
|
Jose Gaston seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Contributor
WalkthroughJest configuration is updated to support 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)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/components/sidebar-toggle-button/__tests__/SidebarToggleButton.test.js (1)
5-11: Consider simplifying the mock setup.The current pattern works but is slightly verbose. You can simplify by using the mock directly without the intermediate require and alias.
jest.mock('elements/common/feature-checking', () => ({ useFeatureConfig: jest.fn(), })); -const { useFeatureConfig } = require('elements/common/feature-checking'); - -const mockUseFeatureConfig = useFeatureConfig; +const { useFeatureConfig: mockUseFeatureConfig } = require('elements/common/feature-checking');Alternatively, if using ES imports is preferred, you could use
jest.mocked()with the import.src/components/sidebar-toggle-button/SidebarToggleButton.js (1)
73-84: Optional: Consider inlining icon selection logic.The
renderIconhelper function (lines 73-78) is only called once and creates a new function instance on each render. For slightly cleaner code, you could inline the icon selection directly:- const renderIcon = () => { - if (direction === DIRECTION_LEFT) { - return isOpen ? <IconShow height={16} width={16} /> : <IconHide height={16} width={16} />; - } - return isOpen ? <IconHide height={16} width={16} /> : <IconShow height={16} width={16} />; - }; - return ( <PlainButton aria-label={intlText} className={classes} onClick={onClick} type="button" {...rest}> - {renderIcon()} + {direction === DIRECTION_LEFT + ? (isOpen ? <IconShow height={16} width={16} /> : <IconHide height={16} width={16} />) + : (isOpen ? <IconHide height={16} width={16} /> : <IconShow height={16} width={16} />)} </PlainButton> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/components/sidebar-toggle-button/__tests__/__snapshots__/SidebarToggleButton.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
scripts/jest/jest.config.js(1 hunks)src/components/sidebar-toggle-button/SidebarToggleButton.js(2 hunks)src/components/sidebar-toggle-button/__tests__/SidebarToggleButton.test.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 (10)
📓 Common learnings
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.
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-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: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
📚 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-06-17T15:21:36.180Z
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 4126
File: scripts/webpack.config.js:72-76
Timestamp: 2025-06-17T15:21:36.180Z
Learning: The Box UI Elements project does not run webpack builds on Windows machines, so Windows path separator compatibility is not a concern for their build scripts.
Applied to files:
scripts/jest/jest.config.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/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-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.flow
🧬 Code graph analysis (2)
src/components/sidebar-toggle-button/__tests__/SidebarToggleButton.test.js (2)
src/elements/common/feature-checking/hooks.js (1)
useFeatureConfig(6-9)src/components/sidebar-toggle-button/SidebarToggleButton.js (1)
SidebarToggleButton(31-92)
src/components/sidebar-toggle-button/SidebarToggleButton.js (3)
src/elements/common/feature-checking/hooks.js (1)
useFeatureConfig(6-9)src/features/collapsible-sidebar/CollapsibleSidebarLogo.js (1)
classes(87-87)src/features/collaborator-avatars/CollaboratorAvatarItem.js (1)
rest(27-36)
⏰ 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 (5)
scripts/jest/jest.config.js (1)
11-11: LGTM!The new
elements/path alias is a clean addition that enables cleaner imports in tests and aligns with the mocking pattern used in the new SidebarToggleButton tests.src/components/sidebar-toggle-button/__tests__/SidebarToggleButton.test.js (1)
48-103: Good test coverage for the feature flag path.The new test suite comprehensively covers:
- Both directions (left/right)
- Both states (open/closed)
- Click handler invocation
- IconButton prop verification
This ensures the feature-flag-driven rendering behaves correctly.
src/components/sidebar-toggle-button/SidebarToggleButton.js (3)
50-71: Verify icon selection logic matches visual chevron directions for both sidebar directions.The icon selection logic is inverted between directions:
- RIGHT:
isOpen ? IconRightSidebarChevronClose : IconRightSidebarChevronOpen- LEFT:
isOpen ? IconRightSidebarChevronOpen : IconRightSidebarChevronCloseSince both directions use icons named "RightSidebarChevron," confirm that this logic correctly produces the intended visual behavior (chevron pointing in the correct direction for open/closed states on both left and right sidebars).
53-69: Verify if IconButton is intentionally designed differently from PlainButton.The review flags missing
classNameand{...rest}props on IconButton compared to PlainButton. Before requesting changes, confirm whether this difference is intentional—IconButton may have a deliberately constrained props interface, or styling/extensibility may be handled differently (e.g., through context or CSS defaults). If parent components currently pass additional props expecting them to propagate, this would be a breaking change requiring fix. Otherwise, the design difference may be acceptable.
7-8: Verify IconButton API compatibility with blueprint-web version 12.7.1.The component uses IconButton from
@box/blueprint-webwith propsaria-label,icon,onClick,size="large", andvariant="high-contrast". Ensure these props are supported in version 12.7.1.
Comment on lines
+13
to
+14
| fill={color} | ||
| fill-opacity="0.6" |
Contributor
There was a problem hiding this comment.
Use camelCase fillOpacity for React/JSX SVG attributes.
In JSX, SVG attributes should use camelCase. fill-opacity is the HTML/SVG DOM attribute name, but React expects fillOpacity.
<path
d="M15.707 9.29297C15.3165 8.90245 14.6835 8.90245 14.293 9.29297C13.9024 9.68349 13.9024 10.3165 14.293 10.707L15.5859 12L14.293 13.293C13.9024 13.6835 13.9024 14.3165 14.293 14.707C14.6835 15.0975 15.3165 15.0975 15.707 14.707L17.707 12.707C18.0975 12.3165 18.0975 11.6835 17.707 11.293L15.707 9.29297Z"
fill={color}
- fill-opacity="0.6"
+ fillOpacity="0.6"
/>
<path
fill-rule="evenodd"
clip-rule="evenodd"
d="M7 4C4.23858 4 2 6.23858 2 9V15C2 17.7614 4.23858 20 7 20H17C19.6753 20 21.8595 17.8989 21.9932 15.2568L22 15V9C22 6.23858 19.7614 4 17 4H7ZM17 6C18.6569 6 20 7.34315 20 9V15L19.9961 15.1543C19.9158 16.7394 18.6051 18 17 18H12V6H17ZM10 18H7C5.34315 18 4 16.6569 4 15V9C4 7.34315 5.34315 6 7 6H10V18Z"
fill={color}
- fill-opacity="0.6"
+ fillOpacity="0.6"
/>Similarly, fill-rule and clip-rule on lines 17-18 should be fillRule and clipRule.
Also applies to: 20-21
🤖 Prompt for AI Agents
In src/icons/general/IconRightSidebarChevronClose.js.flow around lines 13-14
(and similarly lines 17-18 and 20-21), the SVG attributes are using kebab-case
(fill-opacity, fill-rule, clip-rule) which React/JSX requires in camelCase;
update those props to fillOpacity, fillRule, and clipRule respectively so JSX
recognizes them and preserves intended styling.
Comment on lines
+12
to
+21
| fill={color} | ||
| fill-opacity="0.6" | ||
| /> | ||
| <path | ||
| fill-rule="evenodd" | ||
| clip-rule="evenodd" | ||
| d="M7 4C4.23858 4 2 6.23858 2 9V15C2 17.7614 4.23858 20 7 20H17C19.6753 20 21.8595 17.8989 21.9932 15.2568L22 15V9C22 6.23858 19.7614 4 17 4H7ZM17 6C18.6569 6 20 7.34315 20 9V15L19.9961 15.1543C19.9158 16.7394 18.6051 18 17 18H12V6H17ZM10 18H7C5.34315 18 4 16.6569 4 15V9C4 7.34315 5.34315 6 7 6H10V18Z" | ||
| fill={color} | ||
| fill-opacity="0.6" | ||
| /> |
Contributor
There was a problem hiding this comment.
Use camelCase for React/JSX SVG attributes.
React expects camelCase SVG attributes. The hyphenated attributes will cause React warnings and may not apply correctly.
<path
d="M15.707 9.29297C15.3165 8.90245 14.6835 8.90245 14.293 9.29297C13.9024 9.68349 13.9024 10.3165 14.293 10.707L15.5859 12L14.293 13.293C13.9024 13.6835 13.9024 14.3165 14.293 14.707C14.6835 15.0975 15.3165 15.0975 15.707 14.707L17.707 12.707C18.0975 12.3165 18.0975 11.6835 17.707 11.293L15.707 9.29297Z"
fill={color}
- fill-opacity="0.6"
+ fillOpacity="0.6"
/>
<path
- fill-rule="evenodd"
- clip-rule="evenodd"
+ fillRule="evenodd"
+ clipRule="evenodd"
d="M7 4C4.23858 4 2 6.23858 2 9V15C2 17.7614 4.23858 20 7 20H17C19.6753 20 21.8595 17.8989 21.9932 15.2568L22 15V9C22 6.23858 19.7614 4 17 4H7ZM17 6C18.6569 6 20 7.34315 20 9V15L19.9961 15.1543C19.9158 16.7394 18.6051 18 17 18H12V6H17ZM10 18H7C5.34315 18 4 16.6569 4 15V9C4 7.34315 5.34315 6 7 6H10V18Z"
fill={color}
- fill-opacity="0.6"
+ fillOpacity="0.6"
/>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/icons/general/IconRightSidebarChevronClose.tsx around lines 12 to 21 the
SVG uses hyphenated attributes (fill-opacity, fill-rule, clip-rule) which React
expects in camelCase; replace fill-opacity with fillOpacity, fill-rule with
fillRule and clip-rule with clipRule (in both path elements) so the attributes
follow React/JSX conventions and eliminate warnings.
Comment on lines
+13
to
+14
| fill={color} | ||
| fill-opacity="0.6" |
Contributor
There was a problem hiding this comment.
Use camelCase for React/JSX SVG attributes.
Same issue as in IconRightSidebarChevronClose.js.flow: use fillOpacity, fillRule, and clipRule instead of their hyphenated equivalents.
<path
d="M17.707 9.29297C17.3165 8.90245 16.6835 8.90247 16.293 9.29297L14.293 11.293C13.9024 11.6835 13.9024 12.3165 14.293 12.707L16.293 14.707C16.6835 15.0975 17.3165 15.0975 17.707 14.707C18.0975 14.3165 18.0975 13.6835 17.707 13.293L16.4141 12L17.707 10.707C18.0975 10.3165 18.0975 9.68349 17.707 9.29297Z"
fill={color}
- fill-opacity="0.6"
+ fillOpacity="0.6"
/>
<path
- fill-rule="evenodd"
- clip-rule="evenodd"
+ fillRule="evenodd"
+ clipRule="evenodd"
d="M7 4C4.23858 4 2 6.23858 2 9V15C2 17.7614 4.23858 20 7 20H17C19.6753 20 21.8595 17.8989 21.9932 15.2568L22 15V9C22 6.23858 19.7614 4 17 4H7ZM17 6C18.6569 6 20 7.34315 20 9V15L19.9961 15.1543C19.9158 16.7394 18.6051 18 17 18H12V6H17ZM10 18H7C5.34315 18 4 16.6569 4 15V9C4 7.34315 5.34315 6 7 6H10V18Z"
fill={color}
- fill-opacity="0.6"
+ fillOpacity="0.6"
/>Also applies to: 16-21
🤖 Prompt for AI Agents
In src/icons/general/IconRightSidebarChevronOpen.js.flow around lines 13-14 (and
also lines 16-21), the JSX SVG attributes use hyphenated names (e.g.,
fill-opacity, fill-rule, clip-rule); update them to their camelCase React
equivalents (fillOpacity, fillRule, clipRule) throughout the file, keeping the
same values and formatting so the SVG props are valid in React/JSX.
Comment on lines
+12
to
+21
| fill={color} | ||
| fill-opacity="0.6" | ||
| /> | ||
| <path | ||
| fill-rule="evenodd" | ||
| clip-rule="evenodd" | ||
| d="M7 4C4.23858 4 2 6.23858 2 9V15C2 17.7614 4.23858 20 7 20H17C19.6753 20 21.8595 17.8989 21.9932 15.2568L22 15V9C22 6.23858 19.7614 4 17 4H7ZM17 6C18.6569 6 20 7.34315 20 9V15L19.9961 15.1543C19.9158 16.7394 18.6051 18 17 18H12V6H17ZM10 18H7C5.34315 18 4 16.6569 4 15V9C4 7.34315 5.34315 6 7 6H10V18Z" | ||
| fill={color} | ||
| fill-opacity="0.6" | ||
| /> |
Contributor
There was a problem hiding this comment.
Use camelCase for React/JSX SVG attributes.
Same issue as in the other icon components. React expects fillOpacity, fillRule, and clipRule.
<path
d="M17.707 9.29297C17.3165 8.90245 16.6835 8.90247 16.293 9.29297L14.293 11.293C13.9024 11.6835 13.9024 12.3165 14.293 12.707L16.293 14.707C16.6835 15.0975 17.3165 15.0975 17.707 14.707C18.0975 14.3165 18.0975 13.6835 17.707 13.293L16.4141 12L17.707 10.707C18.0975 10.3165 18.0975 9.68349 17.707 9.29297Z"
fill={color}
- fill-opacity="0.6"
+ fillOpacity="0.6"
/>
<path
- fill-rule="evenodd"
- clip-rule="evenodd"
+ fillRule="evenodd"
+ clipRule="evenodd"
d="M7 4C4.23858 4 2 6.23858 2 9V15C2 17.7614 4.23858 20 7 20H17C19.6753 20 21.8595 17.8989 21.9932 15.2568L22 15V9C22 6.23858 19.7614 4 17 4H7ZM17 6C18.6569 6 20 7.34315 20 9V15L19.9961 15.1543C19.9158 16.7394 18.6051 18 17 18H12V6H17ZM10 18H7C5.34315 18 4 16.6569 4 15V9C4 7.34315 5.34315 6 7 6H10V18Z"
fill={color}
- fill-opacity="0.6"
+ fillOpacity="0.6"
/>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/icons/general/IconRightSidebarChevronOpen.tsx around lines 12 to 21 the
SVG props use kebab-case (fill-opacity, fill-rule, clip-rule) which React/JSX
doesn't recognize; change them to camelCase (fillOpacity, fillRule, clipRule)
for each occurrence of those attributes on the <path> elements and keep existing
values intact.
jmcbgaston
changed the title
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.
Summary by CodeRabbit
Release Notes
New Features
Icon Assets
Tests
✏️ Tip: You can customize this high-level summary in your review settings.