Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Add playlist support #6

Merged
merged 4 commits into from
Nov 12, 2021
Merged

WIP: Add playlist support #6

merged 4 commits into from
Nov 12, 2021

Conversation

dkotter
Copy link
Collaborator

@dkotter dkotter commented Nov 10, 2021

Description of the Change

This PR adds basic playlist support to our custom block. I copied the approach that the new core Gallery block uses, specifically around how it uses innerBlocks, only utilizing Audio blocks and not Image blocks.

The initial state looks like this:

Screen Shot 2021-11-11 at 4 17 42 PM

You can then select one or more items from the Media Library and that will render out the individual Audio blocks as well as the WebAmp player:

Screen Shot 2021-11-11 at 4 18 45 PM

You can then remove any of the Audio blocks or add new audio files and everything updates.

On the front-end, currently it renders out all the individual Audio blocks and then those get replaced with the WebAmp player.

There are a number of remaining items that I see here:

  • I had debated on when the WebAmp player should load: if we only show that when the block isn't selected; if we have a preview toggle like some other core blocks use that renders that; some other approach? Right now that will render as soon as media items have been added, whether the block is selected or not. Need to decide if that is the right approach
  • The WinAmp player is meant to be moved around the page, which is fine though does get weird in the admin (I don't see any option to turn this off). But currently having a bigger issue (I'm guessing some styling conflict with core Gutenberg styles) where the player scrolls with the window. This needs fixed but I couldn't figure it out
  • Do we want to style the individual Audio blocks differently? I'd love to have these show up more as a playlist, with song title and artist but currently the Audio block doesn't store that information. This could be a reason to add our own custom block instead of using the core block (otherwise maybe some filtering would be needed)
  • You can currently select multiple audio files but you have to do the command+click or shift+click, otherwise it deselects whatever you have. There is an audio playlist media modal but as far as I can tell, that is not supported yet in the mediaPlaceholder component (you can get to this flow using the Classic block). This may be as good as it gets until support is added for that
  • Drag and drop of media items isn't working properly. They upload fine but it breaks the block
  • Some weirdness between the audio blocks and the WebAmp player, where if you click the start button on the Audio block it seems to start the player instead (which may be fine) and if you remove an Audio block, it sometimes starts playing the next song in the player
  • On the FE, right now it renders out the audio players and then those get replaced with the WebAmp player. Thinking some basic styling here where those players get hidden and maybe some sort of placeholder container, so once the player loads in it's not shifting the page contents around

There's probably other things that could be tweaked/fixed/changed as well that I'm missing. The basic pieces are all there, where you can add the block, you can add one or more songs and that then renders on the FE. But lots of little things we could make better here

Alternate Designs

Instead of using core/audio blocks within an innerBlocks container, we could add our own custom block to handle the playlist items

@dkotter dkotter self-assigned this Nov 10, 2021
@dkotter dkotter linked an issue Nov 10, 2021 that may be closed by this pull request
@jeffpaul jeffpaul requested review from fabiankaegy and dinhtungdu and removed request for fabiankaegy November 11, 2021 04:07
@jeffpaul
Copy link
Member

Ugh not able to set two reviewers it seems... so pinging @fabiankaegy and @dinhtungdu for review and merge as you're ready

@dkotter
Copy link
Collaborator Author

dkotter commented Nov 11, 2021

Note that this is still a work in progress. I'm hoping to detail out tomorrow the approach I took and some of the downsides I'm encountering, as well as the pieces that I still see left here. I also have some questions and areas of feedback I'd love to get. Feel free to review in the meantime though but just want to set proper expectations here.

@dkotter
Copy link
Collaborator Author

dkotter commented Nov 11, 2021

Added more details to the PR now. As I mention in that, I think all core functionality is in place and working but lots of things that could be improved upon. Happy to address any feedback anyone may have or feel free to checkout this branch and work on top of it.

@jeffpaul jeffpaul added this to the 1.0.0 milestone Nov 12, 2021
Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The block works with audio files in the library. But it doesn't work if I upload the media file directly. I think that should be fixed because it's a bug, other enhancements can be implemented later.

@dinhtungdu
Copy link
Contributor

Also, the player position is fixed in the editor. It's due to the position being related to the body, not the editor wrapper. It doesn't happen on the front end.

@jeffpaul
Copy link
Member

I'd say let's resolve the merge conflicts and merge this in. I can work on opening issues for the remaining issues Darin noted as well as what Tung called out, then we can sort out what's a legitimate initial release blocker and what can get fixed up later on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Playlist block support
4 participants