feat(player): scroll wheel on volume slider to adjust volume (#118)#121
Merged
Conversation
Hovering the volume control in the player bar and scrolling the
wheel now nudges the volume by 5 % per tick — wheel up raises,
wheel down lowers, matching the keyboard arrow step that's
already wired up.
Bound directly with `addEventListener('wheel', ..., { passive:
false })` instead of an `onWheel` JSX handler because React 17+
attaches root wheel listeners as passive: a JSX `onWheel` can't
`preventDefault` the underlying page scroll, so the track list
behind the player bar would scroll at the same time. The native
listener with `passive: false` suppresses that cleanly.
The handler sits on the first inner `<div>` that already groups
the volume icon + slider track, so scrolling anywhere over the
visible control area works. Effect deps keep `volume` and
`setVolume` in scope; re-binding on every wheel tick is cheap
(single addEventListener / removeEventListener pair).
InstaZDLL
added
scope: frontend
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughLe composant VolumeControl enregistre un écouteur wheel non-passif sur son conteneur (wheelHostRef) et ajuste l'état volume par pas de 5% selon le signe de deltaY, en appelant preventDefault() pour bloquer le scroll de page; le listener est détaché au démontage. ChangesContrôle du volume à la molette
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/components/player/VolumeControl.tsx`:
- Around line 28-33: Le gestionnaire `handler` considère `e.deltaY === 0` comme
une baisse de volume (car il utilise `e.deltaY < 0 ? WHEEL_STEP : -WHEEL_STEP`),
il faut ignorer ce cas pour éviter une baisse involontaire; modify `handler` in
`VolumeControl.tsx` to early-return when `e.deltaY === 0` (i.e., check `if
(e.deltaY === 0) return;`) before calling `preventDefault()`/`setVolume`, so
only non‑zero vertical wheel deltas change the volume via `setVolume` using
`WHEEL_STEP`.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0c604eea-7c58-477d-ba62-c783d5549e28
📒 Files selected for processing (1)
src/components/player/VolumeControl.tsx
…review) `(e.deltaY < 0 ? WHEEL_STEP : -WHEEL_STEP)` treated `deltaY === 0` as a downward tick. Trackpad horizontal swipes deliver exactly that shape (deltaX != 0, deltaY === 0), so swiping sideways over the volume slider silently dropped the level. Early-return on `deltaY === 0` before `preventDefault` so horizontal scrolls pass through to the parent and only true vertical wheel input changes the volume.
Owner
Author
|
Applied in d9fb661. Real bug — trackpad horizontal swipes (deltaX != 0, deltaY === 0) hit the Fix: |
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
1 participant
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.
Closes #118.
Summary
Hovering the volume control in the player bar and scrolling the wheel now nudges the volume by 5 % per tick — wheel up raises, wheel down lowers. The step matches the keyboard arrow step that's already wired up below in the same component.
Why `addEventListener` instead of `onWheel`
React 17+ attaches root `wheel` listeners as passive, so a JSX `onWheel` handler can't `preventDefault` the underlying page scroll. Without that, scrolling over the slider would also scroll the track list behind the player bar. Binding directly with `addEventListener('wheel', handler, { passive: false })` suppresses the page scroll cleanly. No other wheel handlers in the codebase, so no precedent to follow.
The listener is attached to the first inner `
Test plan
Summary by CodeRabbit
Nouvelles Fonctionnalités
Documentation