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

Adapt to new structure introduced in jQuery UI 1.11.0 #70

Merged
merged 1 commit into from Jul 3, 2014

Conversation

rosenfeld
Copy link
Collaborator

I gave it a try. It builds the assets and they seem to be correctly generated. But I didn't actually test the generated gem yet, while I intend to do it tomorrow. Anyway, there are lots of incompatible changes proposed in this PR to consider.

@rosenfeld
Copy link
Collaborator Author

I've just tested the changes in this PR and they worked for my application after changing the way I required jquery-ui in my assets.

@joliss
Copy link
Member

joliss commented Jul 1, 2014

Thanks!

Hm, the comment at the very bottom of the README re collision with jquery-rails still applies, doesn't it? jquery-ui.js was only removed from jquery-rails last year, and it seems the old jquery-rails version might still be used quite widely.

@joliss
Copy link
Member

joliss commented Jul 1, 2014

Do you think we can/should start using the jquery-ui.js name already?

@skateinmars
Copy link
Collaborator

I think it's been long enough since the renaming in jquery-rails that we can go ahead and use the "proper" file names.
The major version of the gem should be bumped though and the README updated accordingly.

I haven't checked the PR but from what I understand the latest jquery-ui release has a different file hierarchy? In this case this is a good opportunity to change the gem and follow the new naming conventions.

@rosenfeld
Copy link
Collaborator Author

I can't think of an user that would like to keep up-to-date with jQuery UI but not with jQuery... They would be able to use up to jQuery UI 1.10.x anyway. I think the jQuery UI structure scheme change is a good opportunity for breaking backwards compatibility, don't you think?

@joliss
Copy link
Member

joliss commented Jul 1, 2014

Hm, yes. Maybe we can just add a warning not to use jquery-rails <3.0.0.

@rosenfeld
Copy link
Collaborator Author

That makes sense to me

@@ -53,6 +53,7 @@ def map_dependencies
end

DEPENDENCY_HASH = map_dependencies
puts DEPENDENCY_HASH
Copy link
Member

Choose a reason for hiding this comment

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

There is a stray puts here... Could you review the diff line-by-line please and make sure it's exactly the code you intended?

I believe the test app (in the testapp directory) needs updating as well for the new file names.

Let me know when you've re-pushed and I'll take a look again so we can merge this. Thanks again for putting this PR together! :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, will fix that. I haven't noticed the testapp directory before. I'll try to figure out how this is intended to be used...

@rosenfeld
Copy link
Collaborator Author

I moved the constant to a method to avoid reprocessing it for every rake task or command like rake -T.

@rosenfeld
Copy link
Collaborator Author

I've fixed the testapp to work with the recent changes and pushed all changes. I've added db/schema.rb to the testapp's .gitignore as the file didn't exist in the repository and it's created upon rake db:migrate.

joliss added a commit that referenced this pull request Jul 3, 2014
Adapt to new structure introduced in jQuery UI 1.11.0
@joliss joliss merged commit f98e3ba into jquery-ui-rails:master Jul 3, 2014
@joliss
Copy link
Member

joliss commented Jul 3, 2014

Awesome, thanks Rodrigo! I've merged this and released it in version 5.0.0.

@rosenfeld
Copy link
Collaborator Author

Thank you very much, Jo :-)

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