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

Add Last.FM integration #20

Merged
merged 10 commits into from May 20, 2014
Merged

Add Last.FM integration #20

merged 10 commits into from May 20, 2014

Conversation

hdmchl
Copy link

@hdmchl hdmchl commented Mar 23, 2014

I've made a few changes here:

  1. Added Last.FM integration so that users can quickly search for the Artist/Album/Track on Last.FM
  2. Changed up the UX for "show new tracks" vs "show all tracks", so that it is more transparent.
  3. Fixed typos.
  4. If we want people to keep contributing to this project, we need to give fair credit. I made some subtle changes to make the project more contribution-friendly.

Hadi Michel Salem added 8 commits March 23, 2014 10:45
Build in functionality
…tifications' option.

--
The "new tracks" pref wasn't actually looking at all tracks ever played and was confusing UX
There was no way to disable notifications, without closing the app altogether
@hdmchl
Copy link
Author

hdmchl commented Mar 23, 2014

You know what, I think we can make the "Play/Pause => Show notification" preference clearer. I made a general change, but I just realised, I kind of got rid of the play-pause feature.

Let me have another go at that... brb

@citruspi
Copy link
Owner

Hey @hadimichael,

This is some great stuff, but I'd appreciate it if you could split the different changes (i.e. Last.fm integration, UX changes, typo fixes, fair credit, etc.) into separate pull requests.

@citruspi

@citruspi citruspi closed this Mar 23, 2014
@hdmchl
Copy link
Author

hdmchl commented Mar 23, 2014

Fair call.

Edit: Hey @citruspi because the changes I made are not entirely sequential and were all done on master (oops), it's going to be tricky to break it all up. I tried branching it, but it got very messy. I'm open to suggestions, but it looks like it's going to have to come through in one request. Sorry mate.

@citruspi
Copy link
Owner

@hadimichael,

Hmm, I'll take a look at the commits tomorrow and figure out a good way to handle it. I'll reopen this pull request for now.

Either way, thanks for contributing.

@citruspi

@citruspi citruspi reopened this Mar 23, 2014
@hdmchl
Copy link
Author

hdmchl commented Mar 23, 2014

Cool. Thanks. I also went back and fixed up that Preferences change I made. Check out the newest commits here. https://github.com/hadimichael/Spotify-Notifications/commits/master

I can include those in a separate pull-request, immediately after this one goes through.

@hdmchl
Copy link
Author

hdmchl commented Apr 6, 2014

Any changes here?

@citruspi
Copy link
Owner

citruspi commented Apr 9, 2014

@hadimichael,

Not yet, but I do plan to merge the changes. I've just been busy, last week with projects, and this week with PyCon.

Before merging, I just want to actually go through and read the changes to the code.

However, I do have one change I'd like made before I'll merge.

If we want people to keep contributing to this project, we need to give fair credit. I made some subtle changes to make the project more contribution-friendly.

I don't particularly care that you removed my name from it, as I planned to do that anyway, but I disagree that linking to the Contributor Graph is the best way to give fair credit. In the past, I've seen that it takes some time for someone to appear on the Graph, and sometimes they never do. I think that instead you should link to the contributors.md file (located here) which allows people who have contributed in ways other than pull requests to be recognized.

Thoughts?

@citruspi

@citruspi citruspi merged commit 44832dd into citruspi:master May 20, 2014
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

2 participants