BrainzPlayer SPA with Listening Queues #2827
Conversation
There was a problem hiding this comment.
It's working really well !
There are some possible issues with the repeat mode I think, and we are missing to convert a few components to allow for the ambient queue, but overall I would say we can aim to merge this now.
As I suggested below, maybe comment out the repeat mode button for now if you want to take some time to test it?
| // Stop the currently playing song | ||
| if (!playerPaused) { | ||
| await togglePlay(); | ||
| } |
There was a problem hiding this comment.
I'm not sure I understand why the current song should be paused, it's unrelated to wanting to clear thed queue of whatever comes next.
It surprised me that the song was paused when I tried clearing the queue, I thought I broke something and BP crashed.
| // If there is only one item in the queue, and invert is true, play it again | ||
| nextListenIndex = 0; | ||
| } else { | ||
| if (_isEqual(queueRepeatMode, QueueRepeatModes.one)) { |
There was a problem hiding this comment.
I think the repeat modes only work when pressing the next/previous buttons, but not following the end of the song.
In my opinion, this repeat modes feature is not the most the one I find most pressing, so if we have more work on it I would be happy to hide the repeat mode button and work on it in another PR.
I have been wondering if the repeat mode button would make sense inside the queue component instead of the BP bar, but I'm not sure of that. What do you think?
In particular I'm worried about all these buttons on mobile phones, we're going to have to change it because we already can't see the title or artist:
| @@ -478,13 +478,13 @@ export default class RecommendationsPage extends React.Component< | |||
| <RecommendationPlaylistSettings playlist={selectedPlaylist} /> | |||
| </section> | |||
| )} | |||
| <BrainzPlayer | |||
| {/* <BrainzPlayer | |||
There was a problem hiding this comment.
In my opinion, for these few components that are still class components, it might be a good idea to use a wrapper as we do in other places just to be able to use useBrainzPlayerDispatch.
Considering the time it took me to rewrite the UserFeed component and accompanying tests, using a wrapper is our shortest path to deploying the new features (both SPA and the BP improvement here).
What do you think? I know you weren't convinced when we last talked on video.
…nbrainz-server into brainzplayer-spa
…enbrainz-server into brainzplayer-spa
…erver into brainzplayer-spa
This PR enables BrainzPlayer for continuous playback despite navigation. It also adds Listening Queues and Ambient Queues to BrainzPlayer.
Tasks left :
Migrate the following components to functional components
Sync Queues with LocalStorage to ensure queues are synced with other tabs(Followup PR)