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

Adds "Save Song" button to save music to localstorage.Still in need… #212

Closed
wants to merge 9 commits into from

Conversation

scoyne122
Copy link

@scoyne122 scoyne122 commented Jun 9, 2016

Added a "save song" button that saves a song to local storage. Still needs a "my music" section that can display the users saved songs.

localStorage.setItem("mySongs", JSON.stringify(mySongs));
}
});
}
Copy link
Contributor

@eirism eirism Jun 9, 2016

Choose a reason for hiding this comment

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

Insert a comma here and see if it builds (Line 279)

<br>
<br>
Copy link
Member

Choose a reason for hiding this comment

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

why this added break?

@shakeelmohamed
Copy link
Member

@scoyne122 thanks for your efforts here. I think this raises an interesting idea, I've been thinking about doing something similar. The "right" way to implement this would be using the YouTube API (otherwise you'll lose your list when changing browsers or devices) to save videos to a playlist, and require oauth (see what I did for YouTube watch later). This also ties into #1 - adding support for YouTube playlists.

However, it's not quite that simple. When #186 goes in, we'll have Soundcloud support, meaning we'd need to support and cache 2 auth tokens. That's not necessarily a bad thing, but something we should collectively think about. Then we should consider how we want to handle playlists: a) YouTube & Soundcloud playlists are separated, or b) we add our own playlists feature that can contain music from either source. I think option b is more use friendly, but will be trickier to implement.

Let me know what you guys think! cc: @zen-audio-player/contributors

@scoyne122
Copy link
Author

@shakeelmohamed I agree with moving to a cloud based approach to ensure the user's music transfers over devices or browsers. However, assuming Sound Cloud is integrated, it may be easier to implement our own user login system rather than the youtube API with oauth. Perhaps we could create a PHP or node.js backend that connects to a mySQL?

Let me know your thoughts.

@shakeelmohamed
Copy link
Member

Perhaps we could create a PHP or node.js backend that connects to a mySQL?

I've been against this so far in order to keep the project both free and totally open. What you see on the master branch of this repo is exactly what's served on https://ZenPlayer.Audio since we're using GitHub pages. Even if we implemented our own accounts, they would be based on oauth from another platform. I'm not interested in adding a database for this project because of the added cost and complexity. If anyone needs to touch the backend or database there's additional security issues that will arise.

@scoyne122
Copy link
Author

Fair enough.
If we do decide on adding our own playlist feature (which I'm still a fan of), what would be your preferred method of doing so?

@shakeelmohamed
Copy link
Member

We could do something like having a reserved playlist name ("Zen Audio Player") on both YouTube and Soundcloud, then aggregate them on the website depending on which service(s) the user is authenticated.

@YuriBrunetto
Copy link

Hey guys!
@scoyne122 great work 👍

But, as @shakeelmohamed said: I also think that it would be better to create a playlist on YouTube (maybe something with the name like Yuri's ZenAudio Playlist).

@shakeelmohamed
Copy link
Member

@scoyne122 shall we close this PR?

@shakeelmohamed
Copy link
Member

Closing since we've decided this won't be merged. Let's continue this conversation in #229

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.

None yet

4 participants