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

Removed jquery requirement from core.js #30

Merged

Conversation

driv3r
Copy link

@driv3r driv3r commented Jan 29, 2013

I don't think that including jquery in jquery.ui.core is a good idea, because some can get it i.e. from CDN, or require it anyway - it's not mentioned that it's required by UI.

@joliss
Copy link
Member

joliss commented Jan 29, 2013

I like the change, but the Rakefile needs to be updated instead -- see https://github.com/joliss/jquery-ui-rails#hacking-jquery-ui. Changes under vendor would be overwritten with the next update.

Also, don't forget to put a note in the README that you need to require jquery.

@@ -1,5 +1,10 @@
# master

# 3.0.1 [BREAKING CHANGE]
Copy link
Member

Choose a reason for hiding this comment

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

You can omit this line. I'll add a 4.0.0 section when I release.

@joliss
Copy link
Member

joliss commented Jan 29, 2013

Thanks for elucidating, Stuart!

For posterity, original discussion is at #17.

@driv3r
Copy link
Author

driv3r commented Feb 1, 2013

Hey! I think I need to reconfigure the GH settings, I don't have any mail updates :/

Good point about Rakefile, somehow I missed that part. Should I update it?

@stmontgomery
Copy link
Collaborator

Yes please, and remove the existing change to vendor/assets/javascripts/jquery.ui.core.js

@driv3r
Copy link
Author

driv3r commented Feb 1, 2013

Ok - done. I also checked testapp this time, and everything still works ;)

@joliss
Copy link
Member

joliss commented Feb 4, 2013

Sweet. Could you squash the commits please, with git rebase -i?

Stripped Rakefile from jquery dependencies and regenerated assets
@driv3r
Copy link
Author

driv3r commented Feb 4, 2013

Here you go, should be fine now

joliss added a commit that referenced this pull request Feb 4, 2013
Removed jquery requirement from core.js
@joliss joliss merged commit 21e10d7 into jquery-ui-rails:master Feb 4, 2013
@joliss
Copy link
Member

joliss commented Feb 4, 2013

Thanks Leszek! I just pushed out version 4.0.0.

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

3 participants