Feat: Added Volume Control Slider to BrainzPlayer #3097
Conversation
anshg1214
force-pushed
the
volume-slider
branch
from
79787fc to
0cc8fe3
Compare
| return <div data-testid="spotify-player">{this.getAlbumArt()}</div>; | ||
| }} | ||
| </BrainzPlayerContext.Consumer> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Adding a comment here as I cannot comment on this code piece .
While playing a track on spotify you should also change the volume being set here.
There was a problem hiding this comment.
You should also add a listener for volume change, so that if someone changes volume on the spotify app, the changes are reflected here.
There was a problem hiding this comment.
Hi,
I can't think of any way to track changes in volume from the spotify app 🤔
I tried modifying the existing player_state_changed event listener but it doesnt seem to work.
Can you please help me out in this?
Thanks
There was a problem hiding this comment.
While playing a track on spotify you should also change the volume being set here.
Fixed this in the latest commit by putting setVolume in has_player_state_changed event handler. The volume update in spotify happens whenever this event is fired now.
I don't think so we can track the volume change in spotify app to update on brainzplayer for now.
There was a problem hiding this comment.
Here's a question for @anshg1214 : How do you wan to synchronize volume with spotify?
If the volume changes in spotify app and we manage to get the value, should it change the volume for BP?
If you change the volume for BP, should it change the value of spotify app volume?
That seems maybe a bit too intertwined
There was a problem hiding this comment.
Yes. If we can have that feature that'll be great. We do support change handler such that if someone changes track from the spotify app, the states are updated in BP.
However, if spotify does not support this, we can drop this for now.
There was a problem hiding this comment.
You found a good way to do volume setting in componentDidUpdate, albeit with a small prop check required. Nice work on that front!
However I think the UI needs some more work.
The input on the player bar does not look very good on desktop sizes. It is very wide for no reason, and its placement leaves to be desired. Not sure if you looked at it in desktop size, but it's massive and bright colored, it makes it look like a progress bar...
Usually, what I expect from this type of media player is one of two things:
- a volume button, which on hover or click will show a vertical range input above it
- a small horizontal range input like what you see on youtube, spotify, apple music in their respective player controls
Considering we already show the player queue by sliding it up (click the stacked horizontal bars in the player), maybe the first solution makes the most sense in this case. It would also work for smaller screen sizes more easily, as we would only need space for one extra volume icon in the player bar:
Do let me know if you have questions or if you are unsure about anything, better to talk first to figure things out before implementing :)
| const { show } = this.props; | ||
| const { show, volume } = this.props; | ||
| const player = this.appleMusicPlayer; | ||
| if (player) { |
There was a problem hiding this comment.
For each of these calls in componentDidUpdate, you need to check if the volume prop has changed:
| if (player) { | |
| if (prevProps.volume !== volume && player) { |
| const volumeRef = React.useRef<HTMLInputElement>(null); | ||
| const handleVolumeChange = () => { | ||
| dispatch({ | ||
| type: "VOLUME_CHANGE", | ||
| data: volumeRef?.current?.value ?? 100, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
You don't need a ref for this at all:
| const volumeRef = React.useRef<HTMLInputElement>(null); | |
| const handleVolumeChange = () => { | |
| dispatch({ | |
| type: "VOLUME_CHANGE", | |
| data: volumeRef?.current?.value ?? 100, | |
| }); | |
| }; | |
| const handleVolumeChange = (e: React.MouseEvent<HTMLInputElement>) => { | |
| dispatch({ | |
| type: "VOLUME_CHANGE", | |
| data: e.currentTarget?.value ?? 100, | |
| }); | |
| }; |
Don't forget to remove the ref prop on the input element
| @@ -262,6 +263,7 @@ function BrainzPlayerUI(props: React.PropsWithChildren<BrainzPlayerUIProps>) { | |||
| disabled={disabled} | |||
| /> | |||
| </div> | |||
| <VolumeControlButton /> | |||
There was a problem hiding this comment.
For my proposal to turn this into a volume icon that you click/hover on to show the input, you would need to move this one line below.
| }); | ||
| }; | ||
| return ( | ||
| <input |
There was a problem hiding this comment.
Instead of rendering the input directly in here, I suggest rendering a volume icon. When clicking on the icon, the volume input will appear above it.
For CSS styles for the hidden/shown input, you can take inspiration from the player queue (the three horizontal bars in the player UI)
https://github.com/metabrainz/listenbrainz-server/blob/f40c908885229d42c5cb7716ea57a54d39ed2ca4/frontend/css/brainzplayer.less#L354C2-L370C4
Problem
Earlier, ListenBrainz used to play music in max volume by default. This feature aims to let users control the volume of music in ListenBrainz itself.
The feature request ticket was LB-1702.
Solution
I created an input range component. Also added some hooks and states. The UI is basic as of now and can be improved further I believe.
The demo is attached below:
simplescreenrecorder-2024-12-27_17.45.19.mp4
Action
I tested the new feature for YouTube and Spotify but couldn't test it for SoundCloud and Apple Music. I couldn't test it for SoundCloud as they are not accepting new application requests right now so couldn't get the API key. Couldn't test it for Apple Music as I don't have Apple Music premium 😅
If anyone could test it for these two platforms it would be great. Also, please suggest UI changes, if any.
P.S. It's my biggest open-source contribution till date. Thanks @anshg1214 for guiding and supporting me :)