search

freeCodeCamp / CurriculumExpansion

Creative Commons Attribution Share Alike 4.0 International
313 stars 105 forks source link

fix: music player improvements #349

Closed scissorsneedfoodtoo closed 11 months ago

scissorsneedfoodtoo commented 1 year ago

Checklist:

This PR adds a couple of important improvements like playing the next song after the current one is finished. Also, reseting the player state and HTML attributes for accessibility in certain states (the final song finishes playing, songs are shuffled, and so on).

Other changes include reordering the order of the functions, renaming functions for clarity (playNextSong rather than nextSong), and using numbers for song ids instead of strings to simplify the code a bit.

It also adds some comments that may help while breaking the project down into steps.

bbsmooth commented 1 year ago

playing the next song after the current one is finished

This seems like a nice feature to add since that seems like the default behavior for most music players. One concern is that we currently have no way to advance to the end of a song, so in order to test this you have to wait for the entire song to finish. OK, not the end of the world, but somewhat annoying, especially if you want to test it for more than one song. Perhaps we could use just stubs for the songs (like just the first 10-15 seconds of the song) instead of the entire song?

Also, why not make it wrap so that after the last song in the list has played it starts over at the beginning?

scissorsneedfoodtoo commented 1 year ago

Hi @bbsmooth, thanks for your patience and for your helpful feedback.

I just added some short tracks to make it easier to test the feature where the next track is played when the current one is finished.

About the removal of the playback wrapping, we talked about this in a meeting sometime back. Apparently a lot of music players only do that when repeat mode is on. I've only tested this on Spotify and YouTube Music, but that seems to be the case. Also, we were looking for ways to simplify the project slightly. Rafa's code to either play the next / previous track, or wrap around to the first / last song when the next or previous button was pressed worked really well, but seemed pretty complex for the third project in the new JS curriculum.

@JoyShaheb, could you review this and merge this if you're happy with the changes?

RafaelDavisH commented 1 year ago

A minor thing: The dashed outline stays put and does not follow to the next song. It's unsynced with song playing.

scissorsneedfoodtoo commented 11 months ago

@Ksound22 and @larymak reviewed this yesterday and say everything looks good.

Unfortunately they don't have the ability to merge this PR, so I'll go ahead and do it on their behalf so they can continue breaking this project into steps.

bbsmooth commented 11 months ago

The dashed outline stays put and does not follow to the next song. It's unsynced with song playing.

Sorry for the late reply, but I have some reservations about changing this behavior. In general it is not a good idea to change the keyboard focus unless a user initiated action causes the change. In this case, the player is making a change, not the user. The keyboard focus/dashed line does not have to be in sync at all times with the song currently playing in the list. When a song is playing the user is free to move the keyboard focus somewhere other than the song. Forcing it back to the song list when the next song starts seems unnecessary. For example, perhaps they wanted to leave the focus on the next button so they could easily jump to the next song if they didn't like the current one? Granted, with only 10 songs in the play list it wouldn't be much of a hassle to tab back to the next button. But what if there were 50, or 100 songs in the playlist?

Also, this change is causing the keyboard focus to jump to the song in the play list when you use the prev or next buttons at the top, which is definitely not something we want to do. So I guess I am voting that we revert this change and allow the keyboard focus to stay where it is when a song changes.

scissorsneedfoodtoo commented 11 months ago

Thanks for your detailed feedback @bbsmooth. Those are very good points, especially if there's a long playlist and the user wants to keep the focus around one of the control buttons at the top.

I can create a quick PR to revert that. Glad to know that we'll be going back to the expected focus behavior, and be making the project a bit simpler as well.