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

Foundation update #13

Closed
wants to merge 10 commits into from
Closed

Foundation update #13

wants to merge 10 commits into from

Conversation

Glavin001
Copy link
Contributor

Tasks:


Click a track from the list to play it!

url: endpoint,

success: function(data, status, statusText) {
success: function(data, status /*, statusText*/) {
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JSHint was not liking that statusText was defined but not being used. I wanted to keep it there as an argument in case we would use it later and did not want to require a lookup in docs for what the third argument passed would be.

@Glavin001
Copy link
Contributor Author

  • Install global grunt-cli in Travis CI configuration, resolves build failure.

@Glavin001
Copy link
Contributor Author

Thank you for going over my commits, @sindresorhus. Is there anything else you notice that should be amended before merging?

@Glavin001
Copy link
Contributor Author

  • Discuss: should Travis CI test include jshint?

@sindresorhus
Copy link
Member

Discuss: should Travis CI test include jshint?

Yes

@Glavin001
Copy link
Contributor Author

Discuss: should Travis CI test include jshint?

Yes

This is what I originally had by using grunt default as npm test. See comment on issue #11 for details.

I can either change back to what I had: npm test => grunt default, or add another Grunt subtask to test for jshint.

@sindresorhus
Copy link
Member

@Glavin001 what you had was fine. I was just thrown off by the grunt default usage, which is moot since grunt does the same thing.

@Glavin001
Copy link
Contributor Author

I was just thrown off by the grunt default usage, which is moot since grunt does the same thing.

I wanted to be explicit that it was the default task (maybe for Grunt first comers reading the source code, etc) since it would make more sense to be grunt test, however it is not in this case.

@Glavin001 what you had was fine.

I'll change to npm test => grunt.

@Glavin001 Glavin001 mentioned this pull request Feb 20, 2014
3 tasks
@sindresorhus
Copy link
Member

Awesome stuff :)

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.

grunt test Failing Add Travis-CI configuration Main Music Player SoundCloud support
2 participants