-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
create playlist plugin #1018
create playlist plugin #1018
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't want to merge this before "create plugin example and docs". PlaylistParser also could use a better name, why not just call it playlist?
@thijstriemstra yes don't merge until I complete the remaining tasks, I added the label 'needs-update', when I remove the label it will be ready. |
@entonbiba i'm not sure if thats the way to go. maybe you should not make a PR before that checklist is completed? |
I don't have any strong opinions about this. I think we all agree the PR should only be merged when it's ready. But whether PRs should only be opened when they are truly ready (or the author considers them to be ready at least) I don't know – It does add clutter to the PR list and may be confusing if everybody starts doing it. |
I think it's OK to open a PR to discuss the approach before it's too late. Also, is |
@mspae @thijstriemstra @katspaugh yea I opened a pr so I can share the code updates directly here instead of linking to them. I'll remove the other formats for now and create the example for m3u. |
@thijstriemstra @katspaugh @mspae the playlist parser plugin is now ready 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PlaylistParser also could use a better name, why not just call it Playlist or PlayList?
@thijstriemstra sure, all agree on this @katspaugh @mspae ? |
@entonbiba I like that name. |
plugin/wavesurfer.playlistparser.js
Outdated
return this.playlistData; | ||
}, | ||
|
||
parse: function(playlistFile) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parse sounds too techy, I'd personally call it load. And I also prefer playlistData
over parsedList
, can you come up with a better name that avoids parse
?
plugin/wavesurfer.playlistparser.js
Outdated
if (this.playlistType == 'm3u' || this.playlistType == 'audio/mpegurl') { | ||
playlist = playlistFile.replace(/^.*#.*$|#EXTM3U|#EXTINF:/mg, '').split('\n'); | ||
} else { | ||
throw new Error('No valid playlist file provided, valid formats are m3u pls smil json or their valid mime types'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the listing of the supported formats in the error and just keep it short: No valid playlist file provided.
- set getPlaylist method - change parse to loadPlaylist - make generic no valid playlist message
@thijstriemstra updated, I changed it to getPlaylist and loadPlaylist |
create playlist plugin
Parameters:
playlistFile - link to playlist file
playlistType
'm3u' or 'audio/mpegurl' will default to m3u
Preview:
http://codepen.io/entonbiba/pen/OpMQjR