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

Add jQuery version check to existing jQuery presence check #14852

Merged
merged 1 commit into from
Oct 23, 2014

Conversation

hnrch02
Copy link
Collaborator

@hnrch02 hnrch02 commented Oct 22, 2014

Fixes #14809.
Closes #14825.

/cc @cvrebert

@hnrch02 hnrch02 added the js label Oct 22, 2014
@hnrch02 hnrch02 added this to the v3.3.0 milestone Oct 22, 2014
@hnrch02 hnrch02 force-pushed the jquery-version-check branch from 060f78b to 4956ee3 Compare October 22, 2014 18:55
'+function ($) {',
' var versionString = $.fn.jquery.split(\' \')[0]',
' var version = versionString.match(/(\\d+)(?=\\.?)/g)',
' if ((version[0] < 2 && version[1] < 9) || /1\\.9\\.0rc1|1\\.9\\.0b1/.test(versionString)) {',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we could bump the minimum jQuery to 1.9.1 to avoid the RC/Beta complications?

@hnrch02 hnrch02 force-pushed the jquery-version-check branch from 4956ee3 to c6c6dc2 Compare October 23, 2014 03:14
@hnrch02
Copy link
Collaborator Author

hnrch02 commented Oct 23, 2014

@cvrebert Is that better?

@cvrebert
Copy link
Collaborator

Yes. I believe we could also simplify the regex to just split on dots?

@hnrch02 hnrch02 force-pushed the jquery-version-check branch from c6c6dc2 to 1604af0 Compare October 23, 2014 03:57
@hnrch02
Copy link
Collaborator Author

hnrch02 commented Oct 23, 2014

I believe this is as good as it gets 😄

@cvrebert
Copy link
Collaborator

Very nearly. 😄 Why is the .split(' ')[0] necessary?

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Oct 23, 2014

Because of custom builds, see for example GitHub's jQuery build. It identifies itself with 2.1.1 -ajax/jsonp,-deprecated,-exports/amd,-wrap.

@cvrebert
Copy link
Collaborator

LGTM then.

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Oct 23, 2014

Travis failure is unrelated.

hnrch02 added a commit that referenced this pull request Oct 23, 2014
Add jQuery version check to existing jQuery presence check
@hnrch02 hnrch02 merged commit 9814256 into master Oct 23, 2014
@hnrch02 hnrch02 deleted the jquery-version-check branch October 23, 2014 04:15
@hnrch02
Copy link
Collaborator Author

hnrch02 commented Oct 23, 2014

@cvrebert Should I add a ship list entry for this?

@cvrebert
Copy link
Collaborator

Yup!

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Oct 23, 2014

I'm not sure about the section, would you mind adding it for me? 😁

@cvrebert
Copy link
Collaborator

Sure.

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Oct 23, 2014

Merci beaucoup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add jQuery version check to existing jQuery presence check?
2 participants