feat: add range orientation to react components#30
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request adds orientation support to React range components (TimeRange and VolumeRange), allowing them to be rendered vertically or horizontally. The implementation differs from Media Chrome's CSS transform approach by using data attributes and dynamic styling based on the orientation prop.
- Adds orientation prop ('horizontal' | 'vertical') to range components with horizontal as default
- Updates styling to handle both orientations using CSS attribute selectors and dynamic property assignment
- Improves type definitions by replacing generic types with more specific React component prop types
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/react/src/skins/default/styles.ts | Updates CSS classes to support orientation-specific dimensions using data attribute selectors |
| packages/react/react/src/skins/default/MediaSkinDefault.tsx | Sets VolumeRange to use vertical orientation |
| packages/react/react/src/components/VolumeRange.tsx | Adds orientation support with proper type definitions and dynamic styling |
| packages/react/react/src/components/TimeRange.tsx | Adds orientation support with proper type definitions and dynamic styling |
| packages/core/core/src/components/range.ts | Adjusts coordinate calculation for vertical range handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| { x: rect.left, y: rect.top }, | ||
| { x: rect.right, y: rect.bottom } | ||
| { x: rect.left, y: rect.bottom }, | ||
| { x: rect.right, y: rect.top } |
There was a problem hiding this comment.
The coordinate swap appears incorrect for vertical orientation. For a vertical slider, the start point should typically be at the bottom (rect.bottom) and end at the top (rect.top), but the x-coordinates should remain the same (rect.left for both points), not rect.left to rect.right.
| { x: rect.right, y: rect.top } | |
| { x: rect.left, y: rect.top } |
| insetInlineStart: 'var(--slider-fill)', | ||
| [context.orientation === 'horizontal' ? 'insetInlineStart' : 'insetBlockEnd']: 'var(--slider-fill)', | ||
| [context.orientation === 'horizontal' ? 'top' : 'left']: '50%', | ||
| translate: context.orientation === 'horizontal' ? '-50% -50%' : '-50% 50%', |
There was a problem hiding this comment.
The vertical translate value '50%' moves the thumb in the wrong direction. For vertical sliders, the thumb should be centered and translated negatively on both axes. The correct value should be '-50% -50%' for both orientations.
| translate: context.orientation === 'horizontal' ? '-50% -50%' : '-50% 50%', | |
| translate: '-50% -50%', |
| insetInlineStart: 'var(--slider-fill)', | ||
| [context.orientation === 'horizontal' ? 'insetInlineStart' : 'insetBlockEnd']: 'var(--slider-fill)', | ||
| [context.orientation === 'horizontal' ? 'top' : 'left']: '50%', | ||
| translate: context.orientation === 'horizontal' ? '-50% -50%' : '-50% 50%', |
There was a problem hiding this comment.
The vertical translate value '50%' moves the thumb in the wrong direction. For vertical sliders, the thumb should be centered and translated negatively on both axes. The correct value should be '-50% -50%' for both orientations.
| translate: context.orientation === 'horizontal' ? '-50% -50%' : '-50% 50%', | |
| translate: context.orientation === 'horizontal' ? '-50% -50%' : '-50% -50%', |
|
I think Copilot didn't account for the inversion we do for vertical. We stick the progress to the bottom for example. |
Adds the orientation feature to the React range components.
Cleaned up the types as well.
This works differently than in Media Chrome where one would use a CSS rotate transform.
Here you set the orientation prop and then change the width and height on the root and track element.