Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Upgrade to jquery 10 #4681

Merged
merged 20 commits into from Feb 24, 2014

Conversation

Projects
None yet
6 participants
Contributor

fabianrbz commented Jan 11, 2014

ok this is still a WIP, help is much appreciated :)

there are some things that are needed to be done:

  • $.browser was removed, it is recommended in the guides to use http://modernizr.com/ to know the browser but that's overkill imo. something like this should be enough... thoughts?
  • There is an issue with infinitescroll in the contacts page. in this line, changing jQuery.event.handle to jQuery.event.dispatch will do the trick but it's kind of hacky and probably the error is somewhere else because this works in the stream...
  • Test a lot, doing the workarounds mentioned above I was able to have specs and cukes green, but had some failures in jasmine... Probably there are some errors in the app that the tests don't cover :(
Owner

jhass commented Jan 11, 2014

The $.browser workaround would be fine with me, of course the long term goal would be to get rid of browser special casing if possible.

Thanks for working on this, Fabian. I was about to ask whether we should look to upgrade.

I see there's jQuery 2 as well, although that loses support for IE 6, 7 and 8. It's very tempting .... ;)

Owner

Raven24 commented Jan 17, 2014

@goobertron we've never really supported IE < 8 anyway, and IE 8 is getting abandoned as we speak (with support for Win XP ending soon...)

If IE 8 is getting abandoned, can we drop Chrome Frame (the other call to googleapis.com apart from JQuery)?

Contributor

fabianrbz commented Feb 1, 2014

@Raven24 do you think you can give me a hand with the jasmine specs?

Owner

Raven24 commented Feb 2, 2014

sure, ...after this thursday :)

@jhass jhass referenced this pull request Feb 9, 2014

Closed

Updated Jquery-rails #4406

Owner

Raven24 commented Feb 16, 2014

alright, I rebased and fixed the search specs. more to come...

Member

svbergerem commented Feb 16, 2014

How about using jquery-rails 3.1.0? I went through the changelog and it looks like an update could be done without any additional work.

Contributor

fabianrbz commented Feb 18, 2014

@Raven24 thanks for taking a look at this!

Owner

Raven24 commented Feb 18, 2014

@fabianrbz sure :)

... dafuq travis. it hates me. I can feel it

@Raven24 :-) I feel your pain.

Owner

Raven24 commented Feb 18, 2014

yay, jasmine is happy. cucumber is next ...

Contributor

fabianrbz commented Feb 18, 2014

@Raven24 cool! about the cukes... that might be related to the captcha and the firefox's version travis is using... I remember having issues with that because the html5 validations that new browsers have. Captcha validation is not used in the test environment, but the field is still present. -All this is related to the fact that I removed client_side_validations, that's the reason why it doesn't fail in develop-

Owner

Raven24 commented Feb 21, 2014

ok, had to change a few cukes... it might work now ;)

Owner

Raven24 commented Feb 22, 2014

summoning @mrzyx ... any idea for a quick solution for this, maybe?
https://travis-ci.org/diaspora/diaspora/jobs/19392288#L499

Owner

jhass commented Feb 22, 2014

Not the slightest. Iiirc I've the same one in the rails4 branch and hoped you would catch it :P

fabianrbz and others added some commits Jan 4, 2014

Owner

Raven24 commented Feb 23, 2014

hm, my guess is the locale is populated by an earlier test and the 'toEqual' matcher really checks equality in both directions ... so I'm trying clearing the locale for the test runs. Let's see what Travis thinks ;)

Owner

jhass commented Feb 23, 2014

\o/ green travis? Ready to merge?

Member

svbergerem commented Feb 23, 2014

quoting myself:

How about using jquery-rails 3.1.0? I went through the changelog and it looks like an update could be done without any additional work.

note to self: indentation

Owner

jhass replied Feb 23, 2014

:P

Owner

Raven24 commented Feb 23, 2014

I think this PR is already open long enough, and can be merged as is. the main objective is now done and if (hopefully) updating to jquery 11 (gem v3.1) really isn't a big deal, then a separate PR is just a formality.
The most important benefit from this - the actual upgrade of our code and the tests to jquery 10 - should make future jquery updates a breeze.

Member

svbergerem commented Feb 23, 2014

@Raven24 Alright, thanks for your answer. :)

Owner

jhass commented Feb 24, 2014

Alright, let's break stuff :P Thank you @Raven24 and @fabianrbz!

@jhass jhass added a commit that referenced this pull request Feb 24, 2014

@jhass jhass Merge pull request #4681 from diaspora/upgrade_to_jquery_10
Upgrade to jquery 10
a1c222d

@jhass jhass merged commit a2aff72 into develop Feb 24, 2014

1 check passed

default The Travis CI build passed
Details

@jhass jhass added this to the next milestone Feb 24, 2014

Owner

Raven24 commented Feb 24, 2014

as long as we don't do a release immediately we should be fine... ;)

Contributor

fabianrbz commented Feb 24, 2014

@Raven24 thanks! lots of 🍻 for you!

Contributor

Flaburgan commented Feb 24, 2014

Nice work guys! Should I merge this now, or should I wait to see what is happening on smaller pods? There are almost 500 persons on diaspora-fr now, so I'd like to avoid too big regressions...

Owner

Raven24 commented Feb 24, 2014

@Flaburgan well, the tests all pass so the basic functionality should be OK,
otoh, I just noticed a glitch in the publisher, where the mentions overlay is not where it's supposed to be... (on current develop HEAD)

@jhass jhass deleted the upgrade_to_jquery_10 branch Mar 31, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment