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

Jump to bottom

Core: Fixes deprecated calls to jQuery trim. Fixes #2327 #2328

Merged
merged 3 commits into from May 13, 2020
Merged

Core: Fixes deprecated calls to jQuery trim. Fixes #2327 #2328

merged 3 commits into from May 13, 2020

Conversation

brighton1101
Copy link
Contributor

This fixes the issue with deprecation and adds the polyfill for String.prototype.trim for browser support.

n8b
n8b approved these changes May 7, 2020
View reviewed changes
Copy link

@n8b n8b left a comment

Choose a reason for hiding this comment

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

Looks good to me!

staabm
staabm reviewed May 7, 2020
View reviewed changes
src/core.js Outdated
@@ -1,3 +1,12 @@
// JQuery trim is deprecated, polyfill native js method if not defined
if ( !String.prototype.trim ) {
Copy link
Member

Choose a reason for hiding this comment

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

We should not pollute the prototype of builtin objects. Please instead create a local trim-function instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

staabm
staabm reviewed May 7, 2020
View reviewed changes
src/core.js Outdated
@@ -1,3 +1,10 @@
// JQuery trim is deprecated, provide a trim method based on String.prototype.trim
function trim( str ) {
Copy link
Member

Choose a reason for hiding this comment

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

Please move the function into a local variable and move it down to the functions where it is used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

staabm
staabm approved these changes May 7, 2020
View reviewed changes
Copy link
Member

@staabm staabm left a comment

Choose a reason for hiding this comment

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

Lgtm - thanks

@staabm staabm requested a review from Arkni May 7, 2020 19:27
@brighton1101
Copy link
Contributor Author

@staabm @Arkni Any updates? We need to update to JQuery 3.5.1 and depend on this change being made

Arkni
Arkni approved these changes May 12, 2020
View reviewed changes
Copy link
Member

@Arkni Arkni left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks.

@staabm
Copy link
Member

staabm commented May 12, 2020

@Arkni could you take care of merging this and tagging a new release?

@Arkni
Copy link
Member

Arkni commented May 12, 2020

@staabm, I will try my best doing it this night or tomorrow.

@Arkni Arkni merged commit d3748a2 into jquery-validation:master May 13, 2020
@Arkni
Copy link
Member

Arkni commented May 13, 2020

@staabm

My machine is behaving strangely and I couldn't run jquery-release for some reason. It may take me some time to investigate the issue and create a release. Could you please take over the task?

Thank you and sorry for the inconvenience.

@staabm staabm self-assigned this May 14, 2020
@staabm
Copy link
Member

staabm commented May 23, 2020

trying to do a release now

@Arkni
Copy link
Member

Arkni commented May 27, 2020

Thanks a lot @staabm for the release :)

(And sorry for not being active. I'm still in a long pause from a lot of things including open source. It may take me a few more months to be able to help triaging issues or review PRs the way I used to.)

@staabm
Copy link
Member

staabm commented May 27, 2020 •
edited

@Arkni no worries. Take your time - there are a lot of things which are more important then this project.

Best wishes

@bytestream
Copy link
Member

This doesn't resolve all deprecations. Will send another PR

@bytestream
Copy link
Member

Created #2335

@Tandulje
Copy link

Tandulje commented Aug 12, 2020 •
edited

Hi...i'am using jquery -v3.5.1 and jquery.validate -v1.9.2

My code is
var settings = $('form').validate().settings;
settings.rules['UserPrincipalName'].required = true;

it gives error
VM155:294 Uncaught TypeError: Cannot set property 'required' of undefined

When i was on older version jquey -v2.1.4 then it was working fine but now not working with jquey- v3.5.1

Please give some solution on it.
I go through lots of link but not getting related to this.
Please let me know how can i fix this issue.

@bytestream
Copy link
Member

@Tandulje please open a new issue, this is nothing to do with this PR

@Tandulje Tandulje mentioned this pull request Aug 12, 2020
Closed
This was referenced Mar 11, 2021
Merged
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staabm staabm staabm approved these changes

@n8b n8b n8b approved these changes

@Arkni Arkni Arkni approved these changes

Assignees

@staabm staabm

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@brighton1101 @staabm @Arkni @bytestream @Tandulje @n8b