Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds keyboard navigation support to slider components (time and volume sliders), allowing users to control them using arrow keys, Home, and End keys. The implementation includes step size configuration and proper state management for keyboard interactions.
Key Changes:
- Added keyboard event handling to the base
Sliderclass with configurable step sizes - Integrated keyboard controls into
TimeSliderandVolumeSlidercomponents - Modified
Popovercomponent to disable focus management to prevent interference with keyboard navigation
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/core/core/src/components/slider.ts | Added keyboard event listeners, handlers for arrow/Home/End keys, and step size configuration |
| packages/core/core/src/components/time-slider.ts | Extended keyboard handling to trigger seek operations based on keyboard input |
| packages/core/core/src/components/volume-slider.ts | Extended keyboard handling to trigger volume changes and set default step size of 0.1 |
| packages/react/react/src/components/Popover.tsx | Disabled focus management and hover-based initial focus to prevent conflicts with keyboard controls |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| super.handleEvent(event); | ||
|
|
||
| const { _pointerRatio, requestVolumeChange } = super.getState() as VolumeSliderState; | ||
| requestVolumeChange(_pointerRatio); |
There was a problem hiding this comment.
The keyboard event is passed to super.handleEvent(event) which updates _pointerRatio, but then getState() is called immediately after. Due to potential asynchronous state updates, _pointerRatio may not reflect the updated value from the keyboard handler. Consider getting the updated ratio directly from the parent's keyboard handler return value or accessing the updated state differently.
| super.handleEvent(event); | |
| const { _pointerRatio, requestVolumeChange } = super.getState() as VolumeSliderState; | |
| requestVolumeChange(_pointerRatio); | |
| // Compute the new pointer ratio synchronously based on the key event | |
| const state = super.getState() as VolumeSliderState; | |
| let newPointerRatio = state._pointerRatio; | |
| // Example logic: adjust ratio based on arrow keys | |
| if (event.key === 'ArrowLeft' || event.key === 'ArrowDown') { | |
| newPointerRatio = Math.max(0, newPointerRatio - this.getStepSize()); | |
| } else if (event.key === 'ArrowRight' || event.key === 'ArrowUp') { | |
| newPointerRatio = Math.min(1, newPointerRatio + this.getStepSize()); | |
| } | |
| const { requestVolumeChange } = state; | |
| requestVolumeChange(newPointerRatio); | |
| // Optionally, update the pointer ratio in state if needed | |
| // (depends on parent class implementation) | |
| // super.setPointerRatio(newPointerRatio); |
| #handleKeyDown(event: KeyboardEvent) { | ||
| super.handleEvent(event); | ||
|
|
||
| const { _pointerRatio, duration, requestSeek } = super.getState() as TimeSliderState; | ||
|
|
||
| this.#seekingTime = _pointerRatio * duration; | ||
| requestSeek(this.#seekingTime); | ||
| } |
There was a problem hiding this comment.
Same issue as in VolumeSlider: calling getState() immediately after super.handleEvent(event) may not capture the updated _pointerRatio value if state updates are asynchronous. This aligns with the PR description's noted bug about 'relative value bug in time slider and volume slider'.
Uh oh!
There was an error while loading. Please reload this page.