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

jQuery UI upgrade to 1.12 #105

Merged
merged 6 commits into from Nov 29, 2016
Merged

Conversation

Borzik
Copy link
Contributor

@Borzik Borzik commented Sep 25, 2016

Since directory structure has changed in 1.12, previous build system didn't work for 1.12.
jQuery UI doesn't have build info files, so there's no optimal way to get dependency list for each file. I ended up with parsing define([...deps], factory) block of each JS file.

As a result:

  • we completely follow jQuery UI 1.12 folder structure (only ui subfolder has been dropped for usage simplicity)
  • widgets, i18n and effects now have their own directories
  • gem version went up to 6.0.0, since this is a breaking change for most users, and since we upgrade to 1.12
  • also I had to upgrade Travis Ruby version to 2.2.2 because 1.9.3 doesn't work with your gemspec anymore

I've also updated readme.md to follow new directory structure.

Please let me know any questions you have.

And it closes #103

Actually it depends on actionpack 5.0, which is dependent on Rack 2.0, which is not compatible with ruby 1.9.3, and it causes all recent builds to fail
@scarroll32
Copy link

Please merge this :-)

@ttrmw
Copy link

ttrmw commented Oct 24, 2016

+1

2 similar comments
@hunterlong
Copy link

+1

@okamos
Copy link

okamos commented Nov 21, 2016

+1

@joliss
Copy link
Member

joliss commented Nov 28, 2016

Thanks for the PR! Some comments:

  • I cannot seem to install the "json" gem with Ruby 2.3. I've had to update the dependency like so:
diff --git a/jquery-ui-rails.gemspec b/jquery-ui-rails.gemspec
index 4ba6e19..43d58e9 100644
--- a/jquery-ui-rails.gemspec
+++ b/jquery-ui-rails.gemspec
@@ -15,7 +15,7 @@ Gem::Specification.new do |s|

   s.add_dependency "railties", ">= 3.2.16"

-  s.add_development_dependency "json", "~> 1.7"
+  s.add_development_dependency "json", "~> 2.0.2"

   s.files        = `git ls-files`.split("\n").reject { |f| f =~ /^testapp|^jquery-ui/ }
   s.executables  = `git ls-files -- bin/*`.split("\n").map { |f| File.basename(f) }
  • When I merge your PR and run rake, it leaves app/assets/javascripts/jquery-ui/widgets/ empty and git status shows a bunch of files changed. I'd expect it to just regenerate everything in app/assets without change. Never mind this, I forgot to run git submodule update.

@joliss
Copy link
Member

joliss commented Nov 28, 2016

  • Some of your commits are just fixes to earlier commits. Can you use git rebase -i and squash them accordingly, please?

//= require jquery-ui/tabs
//= require jquery-ui/tooltip
//= require jquery-ui/widget
//= require jquery-ui/core.js
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there should be a trailing .js here, should there?

@Borzik
Copy link
Contributor Author

Borzik commented Nov 29, 2016

@joliss thanks for review!
Re: JSON gem: initial gemspec worked for me when I tested everything, but anyway, I've updated it with the code you had provided.
Re: trailing .js extensions: I've missed those somehow. Pushing a fix.
Re: commits that are fixes: it seems like you can squash all commits from PR together on merge. If you still think it would be better if I did that — I'll have to close this PR and create a new one from another branch, because the source branch is being used by me in some projects already, and someone else could have used that as well.

@joliss joliss merged commit 82e6c6e into jquery-ui-rails:master Nov 29, 2016
@joliss
Copy link
Member

joliss commented Nov 29, 2016

Great, thanks! I've released v6.0.0 of this gem.

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.

Upgrading to v1.12.0
6 participants