feat(content-sidebar): add drag-to-resize handle behind feature flag#4559
Open
jmcbgaston wants to merge 5 commits into
Open
feat(content-sidebar): add drag-to-resize handle behind feature flag#4559jmcbgaston wants to merge 5 commits into
jmcbgaston wants to merge 5 commits into
Conversation
Adds a pointer- and keyboard-accessible resize handle on the left edge of the sidebar. Current default width becomes the minimum; maximum is clamped at 50% of the viewport. Width state lives on the `Sidebar` component and is session-only (no persistence). The new `SidebarResizeHandle` component is a `role="separator"` with live `aria-valuenow` / `aria-valuemin` / `aria-valuemax`, supports ArrowLeft / ArrowRight / Home / End for keyboard resize, and uses pointer capture during drag. Gated by `isFeatureEnabled(features, 'contentSidebar.resizable.enabled')` and additionally restricted to large / x-large viewports via the existing `withMediaQuery` HOC — small / medium viewports keep the original bottom-sheet layout. SCSS overrides for `.bcs-content` and `.bcs-activity-feed` are scoped under `.bcs-is-resizable` so flag-off behavior is unchanged.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a pointer-driven SidebarResizeHandle, wires resizable behavior behind a feature flag and viewport check in Sidebar, stores and clamps user width in state, applies inline width/maxWidth after user resize, and updates SCSS so sidebar content and activity feed fill the resized container. ChangesResizable Sidebar Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/elements/content-sidebar/SidebarResizeHandle.js`:
- Around line 39-64: The Flow errors come from TypeScript event types and
optional-call syntax; update handlePointerUp, handlePointerDown and
handleKeyDown to use Flow's SyntheticPointerEvent and SyntheticKeyboardEvent
types (instead of React.PointerEvent/React.KeyboardEvent) and replace every
optional-call like target.hasPointerCapture?.(...) and
event.currentTarget.setPointerCapture?.(...) and onResizeStart?.() /
onResizeEnd?.() with explicit null/undefined checks (e.g., if (target && typeof
target.hasPointerCapture === 'function')
target.hasPointerCapture(event.pointerId); and if (onResizeStart)
onResizeStart();). Ensure startXRef/current, startWidthRef/current,
setIsDragging, handlePointerMove remain unchanged but used with the new
Flow-compatible event types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02b0b1ec-8256-4d64-b578-9830c44e20d1
📒 Files selected for processing (6)
src/elements/content-sidebar/ContentSidebar.scsssrc/elements/content-sidebar/Sidebar.jssrc/elements/content-sidebar/SidebarContent.scsssrc/elements/content-sidebar/SidebarResizeHandle.jssrc/elements/content-sidebar/SidebarResizeHandle.scsssrc/elements/content-sidebar/activity-feed/activity-feed/ActivityFeed.scss
- Make `maxWidth` reactive by reading `viewWidth` from withMediaQuery (previously read `window.innerWidth` at render time, leaving the cap stale across browser resizes) - Drop unused `onResizeStart` / `onResizeEnd` props from `SidebarResizeHandle` — no caller passes them - Simplify SCSS selectors: `.bcs-is-resizable.bcs-is-wider .bcs-content` was redundant with `.bcs-is-resizable .bcs-content` (same specificity, source order wins) - Replace TS-style React event types with Flow's Synthetic* and replace optional-chain calls with explicit guards so Flow typecheck passes - Strip keyboard interaction for MVP; mark the handle `aria-hidden="true"` so AT users aren't shown an unfocusable widget - Add `SidebarResizeHandle.test.js` covering render, drag-grow, drag-shrink, min/max clamping, dragging class, and listener cleanup - Add a `resizable` describe block to `Sidebar.test.js` covering FF gate, viewport gate, open-state gate, inline-width application, and maxWidth-from-viewport behavior
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/elements/content-sidebar/SidebarResizeHandle.js`:
- Around line 58-60: Flow is complaining because setPointerCapture (called on
event.currentTarget in SidebarResizeHandle.js) is typed to accept a string while
event.pointerId is a number; fix by adding a narrow type assertion/cast so Flow
accepts the runtime numeric pointerId (e.g. coerce event.pointerId to any or the
appropriate expected type) before calling event.currentTarget.setPointerCapture,
leaving the runtime value unchanged and keeping the guard that setPointerCapture
exists.
- Around line 38-46: The runtime guards using typeof on event.target should be
replaced by an instanceof Element check to satisfy Flow's typing: in the pointer
event handler inside SidebarResizeHandle (where event.target, event.pointerId,
hasPointerCapture and releasePointerCapture are used), first assert that
event.target instanceof Element, then call
target.hasPointerCapture(event.pointerId) and
target.releasePointerCapture(event.pointerId) — removing the typeof checks so
Flow knows target is an Element before calling those methods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07925335-2612-4e8b-9552-cd6af9208e25
📒 Files selected for processing (7)
src/elements/content-sidebar/Sidebar.jssrc/elements/content-sidebar/SidebarContent.scsssrc/elements/content-sidebar/SidebarResizeHandle.jssrc/elements/content-sidebar/SidebarResizeHandle.scsssrc/elements/content-sidebar/__tests__/Sidebar.test.jssrc/elements/content-sidebar/__tests__/SidebarResizeHandle.test.jssrc/elements/content-sidebar/activity-feed/activity-feed/ActivityFeed.scss
✅ Files skipped from review due to trivial changes (1)
- src/elements/content-sidebar/SidebarContent.scss
Flow's lib doesn't refine `EventTarget` to a type with pointer-capture methods, so `typeof === 'function'` checks weren't enough. Switch to an `instanceof Element` refinement (which Flow understands) for `releasePointerCapture`, with a small `(target: any)` escape for `hasPointerCapture` — Flow's `Element` lib declares the former but not the latter. Also cast `event.pointerId` (number) to string when passing to the pointer-capture methods: Flow types them as `(string)` even though the real DOM API takes a number. Keep the `typeof setPointerCapture === 'function'` guard at runtime — jsdom doesn't implement it, so dropping the guard breaks unit tests.
|
|
||
| &:hover, | ||
| &.bcs-resize-handle-is-dragging { | ||
| background-color: $bdl-box-blue; |
Contributor
There was a problem hiding this comment.
can we use Blueprint here?
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
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.
Description
Adds a pointer-driven drag-to-resize handle on the left edge of the
ContentSidebar. The current default width becomes the minimum; users can grow the sidebar up to 50% of the viewport. Width is session-only (no persistence) — resets on reload.The new
SidebarResizeHandleis a mouse-only widget markedaria-hidden="true"(keyboard support deferred to a follow-up). It uses pointer capture during drag.Gating:
isFeatureEnabled(features, 'contentSidebar.resizable.enabled')— flag-off is a no-oplarge/xlargeviewports via the existingwithMediaQueryHOC. Small / medium viewports keep the original bottom-sheet layout..bcs-contentand.bcs-activity-feedare scoped under.bcs-is-resizableso flag-off behavior is unchanged.Reflow of the preview viewer happens automatically via the existing
react-measure→preview.resize()chain — no new wiring needed.Test plan
features.contentSidebar.resizable.enabled = true, the handle renders on the left edge of the sidebarfeatures.contentSidebar.resizable.enabled = false, no handle renders and sidebar widths are unchanged≤ 767pxviewport, no handle renders and sidebar collapses to bottom-sheet layoutSemantic release type
feat- New featureSummary by CodeRabbit
New Features
Style