search

davicorreiajr / spotify-now-playing

Spotify now playing information and control popup for macOS menu bar
MIT License
216 stars 11 forks source link

Feature: Notifications when songs change + Shuffle #22

Closed bressan3 closed 5 years ago

bressan3 commented 5 years ago

What does this PR do?

Adds the option to enable/disable shuffle:

screen shot 2018-12-24 at 3 03 33 am

Adds the option to turn on notifications upon song changes:

50385976-84ed9f00-06c5-11e9-9d84-989002a35b4d

When clicked on, notifications bring up Spotify.

Right click on the app's task bar icon and select the "Activate Notifications" option to have them appear when songs change on Spotify.

The "Activate Notifications" option should be set off by default.

This feature uses node-notifier (notifications) and electron-store (to store the user's notification preferences).

How to test?

Make sure that the following line is in your package.json file otherwise notifications won't show up when executing the binary generated by electron via the "yarn dist" command:

"asarUnpack": ["./node_modules/node-notifier/vendor/**"]

davicorreiajr commented 5 years ago

Ahh another thing I forgot to say. Could you check the lint next time? I think if you set your editor to use the .eslintrc will help you

bressan3 commented 5 years ago

Hello Davi and thank you for your comments on this pull request. I completely understand that you want to keep the code as clean as possible and this is something I want to do as well so don't worry :). I've made the proper changes on my code and it's now using nconf like you suggested. Since I had some questions concerning the points you brought up on what you wrote about the way I implemented the shuffle button, I'll wait until I get those clarified to commit the changes I made. And oh, those .swp files you mentioned are automatically generated by the editor I use (Vim). I'll try to leave them all out from my next commit.

davicorreiajr commented 5 years ago

@bressan3 cool! I answered now, if there is still something, just tell me.

bressan3 commented 5 years ago

I've just committed and pushed the changes I made. I still have to do some further testing on this version of the app. The previous version was acting funky after being open for a while or after the computer would wake up from a long sleep period. For some reason te app wouldn't load Spotify's info and would keep trying to fetch it non stop.

Do you know if this has to do with not setting the SENTRY_DSN parameter on .env.json? Because I've been leaving it empty when building the app.

davicorreiajr commented 5 years ago

@bressan3 perfect! I'm gonna merge this, add some stuff related to the Sentry, fix somethings related to the lint and then release. Thank you so much for contributing again!! :)