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

Fix bugs with old jquery version and textlimit alert #93

Merged
merged 3 commits into from Jul 10, 2017

Conversation

Projects
None yet
2 participants
@vkarppinen
Contributor

vkarppinen commented Jul 4, 2017

  • Fixed textcount.js support jquery>1.6.
  • Fixed a bug (that it was possible to enter text length over maxlimit) by replacing maxlimit alert() with highlighting textcountfield.[vkarppinen]
@mauritsvanrees

Seems okay mostly, but I don't know how to test it. I tried with PloneFormGen on Plone 5.0, because you can set maximum characters on fields, but there it actually simply sets a maxlength="8" in the input, and lets the browser take care of it.
So I wonder if we can't just remove it. But yes, if we still ship with this script, it should work with more current jQuery versions.

Show outdated Hide outdated Products/Archetypes/skins/archetypes/widgets/js/textcount.js
@@ -1,12 +1,13 @@
<!-- Original: Ronnie T. Moore -->
<!-- Dynamic 'fix' by: Nannette Thacker -->
function textCounter(field, countfield, maxlimit) {
var fieldval = jQuery(field).attr('value');
var fieldval = jQuery(field).val();
var countfield = jQuery('input[name="' + countfield + '"]');

This comment has been minimized.

@mauritsvanrees

mauritsvanrees Jul 5, 2017

Member

jshint tells me that countfield is already defined, because it is passed into the function. So you don't need the var.

@mauritsvanrees

mauritsvanrees Jul 5, 2017

Member

jshint tells me that countfield is already defined, because it is passed into the function. So you don't need the var.

This comment has been minimized.

@mauritsvanrees

mauritsvanrees Jul 5, 2017

Member

Actually, why are you replacing the countfield that is passed in?

@mauritsvanrees

mauritsvanrees Jul 5, 2017

Member

Actually, why are you replacing the countfield that is passed in?

This comment has been minimized.

@vkarppinen

vkarppinen Jul 6, 2017

Contributor

@mauritsvanrees The "countfield" that is passed to the function is actually name attribute value of the countfield element. Therefore, I create a variable that holds the real element, to get it's values etc.

The naming of the variable is not good, I will change this.

@vkarppinen

vkarppinen Jul 6, 2017

Contributor

@mauritsvanrees The "countfield" that is passed to the function is actually name attribute value of the countfield element. Therefore, I create a variable that holds the real element, to get it's values etc.

The naming of the variable is not good, I will change this.

Show outdated Hide outdated Products/Archetypes/skins/archetypes/widgets/js/textcount.js
@@ -1,12 +1,13 @@
<!-- Original: Ronnie T. Moore -->
<!-- Dynamic 'fix' by: Nannette Thacker -->

This comment has been minimized.

@mauritsvanrees

mauritsvanrees Jul 5, 2017

Member

Since you are touching this file anyway, could you replace these html comments by javascript comments?

@mauritsvanrees

mauritsvanrees Jul 5, 2017

Member

Since you are touching this file anyway, could you replace these html comments by javascript comments?

This comment has been minimized.

@vkarppinen

vkarppinen Jul 6, 2017

Contributor

@mauritsvanrees I will do just that.

@vkarppinen

vkarppinen Jul 6, 2017

Contributor

@mauritsvanrees I will do just that.

Show outdated Hide outdated Products/Archetypes/skins/archetypes/widgets/js/textcount.js
countfield.css('border', '1px solid red');
}
// update 'characters left' counter
countfield.attr('value', Math.max(maxlimit - fieldval.length, 0));

This comment has been minimized.

@mauritsvanrees

mauritsvanrees Jul 5, 2017

Member

A few lines up you replace jQuery(...).attr('value', '...') with jQuery(...).val('...'). Shouldn't you do the same here?
Note: I have no idea if there are cases where one works and the other doesn't.

@mauritsvanrees

mauritsvanrees Jul 5, 2017

Member

A few lines up you replace jQuery(...).attr('value', '...') with jQuery(...).val('...'). Shouldn't you do the same here?
Note: I have no idea if there are cases where one works and the other doesn't.

This comment has been minimized.

@vkarppinen

vkarppinen Jul 6, 2017

Contributor

@mauritsvanrees Yes, I should do the same here. I will change this.

@vkarppinen

vkarppinen Jul 6, 2017

Contributor

@mauritsvanrees Yes, I should do the same here. I will change this.

@mauritsvanrees mauritsvanrees merged commit 4b46995 into master Jul 10, 2017

5 checks passed

Changelog verifier Entry found
Details
Plone Contributors Agreement verifier All users have signed it
Details
Plone Jenkins CI - pull-request-5.1 Job finished with success status
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mauritsvanrees mauritsvanrees deleted the vkarppinen/textcount branch Jul 10, 2017

@mauritsvanrees

This comment has been minimized.

Show comment
Hide comment
@mauritsvanrees

mauritsvanrees Jul 10, 2017

Member

Merged. Thank you!

Member

mauritsvanrees commented Jul 10, 2017

Merged. Thank you!

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