search

helloitszak / bandcamp_volume

A chrome extension to add a volume slider to Bandcamp.com's music player.
31 stars 6 forks source link

v2.0 Update #7

Closed josh-bridge closed 9 years ago

josh-bridge commented 9 years ago

Version 2.0

I saw that you said in your readme that you were just trying to brush up on your js/css skills so I commented the sh*t out of it so you can understand everything I've done, should you want to edit it now or in the future. (Although this is my first chrome extension / major js project so there's bound to be things I didn't do due to lack of knowledge)

I wouldn't say you need to edit anything right now though, I've set up everything that needs to be set up; all you need to do is merge & push the update to the chrome store. (I'd advise at least giving it a once-over before you do that to make sure I didn't make any glaring mistakes)

Obviously if you want to test it, please do. I already have extensively tested it, so don't worry about it if you don't want to. Any questions or things you want to change just let me know and I'll see what I can do. Thanks!

helloitszak commented 9 years ago

Holy balls this is awesome! Thank you for the contribution!

Only issue I have is the use of Bandcamp’s logo. I’d love a new icon for the project, but I don’t want to get into any legal stinks by using their logo without permission. Perhaps just remove their logo from the one you already made?

I should have some time later this week to test everything out, once I have a change to test I’ll get this merged and up on the store as soon as I can.

josh-bridge commented 9 years ago

Technically I don't think there would be any legal trouble with it; I was kind of unhappy with that logo anyway so the new one I just pushed I think is much better and cleaner, and still has the recognisable bandcamp blue/teal

josh-bridge commented 9 years ago

Also, once you merge it, it might be a good idea to add me as a collaborator so I can make any changes / fix any bugs that get submitted if you don't have time / don't want to. As I (obviously) will know the code I wrote and how it works more than you :)

josh-bridge commented 9 years ago

Any updates?

helloitszak commented 9 years ago

Hey sorry about the delay, I only really have hack time on the weekend right now and I was out of town last weekend.

I should be able to get to this in the next couple of days.

josh-bridge commented 9 years ago

Any updates? If I'm honest you really don't need to test it, just merge it and if you have any issues feel free to revert it

helloitszak commented 9 years ago

Alright I took a look and have a couple pieces of feedback.

  1. I'm not sure I feel about the tab sync feature. I can imagine a user might want to mute one song but not have it propagate to other tabs.
  2. The syncing code is very very cluttered. I encountered a bug when troubleshooting where somehow my chrome.storage.sync variables got set to a string undefined and it wouldn't persist anything.

I'm gonna merge but I won't be publishing the next version right away. Gonna do a bit of code cleanup myself. I'll comment again here once the new version goes up.

Thanks again for the contribution!

helloitszak commented 9 years ago

Nixed the sync between tabs feature. Just sent out the publish request to Google.

josh-bridge commented 9 years ago

Woah dude hold your horses! I made an options page so you can toggle the sync between tabs whether you want it or not!