Composable Components #183
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…fset` logic from `setPositionFromBounds` since keyboard events don't use it now
|
Size Change: +2.48 kB (+37.75%) 🚨 Total Size: 9.07 kB
|
…storybook can generate prop tables properly
commit: |
…wn` to `onHandleRootKeyDown` add custom slider and contxt tests
|
There was a problem hiding this comment.
Pull request overview
This pull request implements a major architectural change to introduce composable components for the react-compare-slider library, addressing issue #180. The changes allow developers to build custom sliders using individual exported components and hooks, while maintaining backward compatibility with the existing monolithic ReactCompareSlider component.
Changes:
- Introduced composable component architecture with individual exports for
Provider,Root,Item,HandleRoot,Handle, andImagecomponents - Added
useReactCompareSliderhook to manage slider state and event handlers, anduseReactCompareSliderContextfor accessing slider context - Renamed
positionprop todefaultPositionand changedclipvalues from 'both' to 'all' for consistency - Migrated build tooling from tsup to tsdown with enhanced package validation via attw and publint
- Removed deprecated
useReactCompareSliderRefhook in favor of the new composable API - Updated all documentation, tests, and examples to reflect the new API
Reviewed changes
Copilot reviewed 70 out of 71 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/src/types.ts | Renamed types and restructured props for composable architecture |
| lib/src/consts.ts | Updated clip option naming and added new CSS variables |
| lib/src/hooks.ts | New hook providing complete slider control with state and event handlers |
| lib/src/components/* | New composable component implementations (Provider, Root, Item, HandleRoot, Handle, Image, internal-hooks) |
| lib/src/react-compare-slider.tsx | Refactored main component to use new composable components internally |
| lib/src/index.ts | Updated exports to expose new components and hooks |
| lib/tsdown.config.ts | New build configuration replacing tsup |
| lib/package.json | Updated exports for multi-entry point support and dependency versions |
| docs/storybook/content/stories/* | Updated all stories and tests for new API |
| docs/example/src/App.tsx | Demonstrates both traditional and composable API usage |
| package.json | Updated dependency versions and pnpm configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const isIncrement = portrait | ||
| ? ev.key === KeyboardEventKeys.ARROW_LEFT || ev.key === KeyboardEventKeys.ARROW_DOWN | ||
| : ev.key === KeyboardEventKeys.ARROW_RIGHT || ev.key === KeyboardEventKeys.ARROW_UP; |
There was a problem hiding this comment.
The keyboard navigation logic appears inverted for portrait mode. In portrait mode, ARROW_LEFT and ARROW_DOWN should move the handle down (increase position), but ARROW_DOWN typically means "move down" which should increase the position. However, the current logic treats ARROW_DOWN as an increment, which may be counterintuitive. Consider if ARROW_UP should increase position (move divider up) and ARROW_DOWN should decrease position (move divider down) in portrait mode for better UX.
| const context = useContext(Context); | ||
|
|
||
| if (!context) { | ||
| throw new Error('useReactCompareSliderContext must be used within the Context component'); |
There was a problem hiding this comment.
The error message says "must be used within the Context component" but it should say "must be used within the Provider component" to match the actual component name that developers will use. The test at docs/storybook/content/stories/99-tests/context.test.stories.tsx:44 also expects the old error message and will need to be updated accordingly.
| return () => { | ||
| containerRef.removeEventListener('pointermove', onPointerMove); | ||
| containerRef.removeEventListener('pointerleave', handlePointerLeave); | ||
| }; |
There was a problem hiding this comment.
In the cleanup function, rootRef.current might be null when the component unmounts, but the code doesn't check for this before calling removeEventListener. Although removeEventListener on undefined/null may not throw, it's safer to add a null check: if (containerRef) { containerRef.removeEventListener(...) }.
| /** Optional styles for handle the button. */ | ||
| buttonStyle?: CSSProperties; |
There was a problem hiding this comment.
There's a typo in the JSDoc comment: "Optional styles for handle the button" should be "Optional styles for the handle button" (remove the duplicate "the").
| /** | ||
| * @NOTE These must reflect the default values defined in the `types.ts`. | ||
| */ | ||
| export const args: ReactCompareSliderProps = { |
There was a problem hiding this comment.
The args object uses ReactCompareSliderProps as its type, which marks itemOne and itemTwo as required (non-optional) properties according to lib/src/types.ts. However, this object sets them to undefined which will cause a TypeScript error. Consider using Partial<ReactCompareSliderProps> for the type annotation or handling these required props differently.
| export type { ReactCompareSliderDetailedProps, ReactCompareSliderProps } from './types'; | ||
| export { styleFitContainer } from './utils'; |
There was a problem hiding this comment.
The ReactCompareSliderPosition type is defined in types.ts but is not exported from the main index.ts. If this type is intended to be part of the public API (as it was previously with ReactCompareSliderPropPosition), it should be exported. Otherwise, consumers who want to type position values will need to use number directly.
| const onPointerUp = useCallback((ev: PointerEvent) => { | ||
| setIsDragging(false); | ||
| setCanTransition(true); | ||
| }, []); | ||
|
|
||
| const onTouchEnd = useCallback((ev: TouchEvent) => { | ||
| setIsDragging(false); | ||
| setCanTransition(true); | ||
| }, []); |
There was a problem hiding this comment.
The onPointerUp and onTouchEnd event handlers receive an event parameter but don't use it. Consider adding _ prefix to the parameter name to indicate it's intentionally unused: (ev: PointerEvent) → (_ev: PointerEvent) or remove the parameter entirely if not needed.
| }, [onPointerMove, onPointerUp, isDragging, browsingContext, hasBrowsingContextBinding]); | ||
|
|
||
| return ( | ||
| <div ref={rootRef} style={appliedStyle} data-rcs="root" data-rcs-clip={clip} data-testaroo {...props} /> |
There was a problem hiding this comment.
There's a leftover test attribute data-testaroo in the production code. This should be removed as it appears to be a testing artifact.
| @@ -0,0 +1,56 @@ | |||
| import type { Meta, StoryFn } from '@storybook/react-vite'; | |||
| import { type ReactCompareSlider, useReactCompareSliderContext } from 'react-compare-slider'; | |||
There was a problem hiding this comment.
Unused import useReactCompareSliderContext.
| import { type ReactCompareSlider, useReactCompareSliderContext } from 'react-compare-slider'; | |
| import { type ReactCompareSlider } from 'react-compare-slider'; |
| } | ||
| ref={reactCompareSliderRef} | ||
| browsingContext={browsingContext} | ||
| browsingContext={browsingContext || undefined} |
There was a problem hiding this comment.
This use of variable 'browsingContext' always evaluates to true.
| browsingContext={browsingContext || undefined} | |
| browsingContext={browsingContext} |
#180
useReactCompareSliderRefuseReactCompareSliderhook to return all the props and logic needed by the componentsuseReactCompareSliderContexthook for components within the context provider to access props