.css(), the values like +=10% aren't ignored anymore (like .animate()) #2011

mr21 · 2015-01-13T19:03:06Z

Hi :)

Actually when we make:

$element.animate({width: "+=10%"}, 500);
it's work fine, the value is converted into percentage BUT if we write:

$element.css("width", "+=10%");
We don't have the same behavior...
The .css() function doesn't look the unity, it's "px" every time.

The deletion of my two old commit has closed my last pull request... and even when I make another new commit I can't reopen it, sorry for that doublon.

So, @gibson042 told me to see this part: https://github.com/jquery/jquery/blob/master/src/effects.js#L38-L60
Before, I didn't understand I could make a new CSS change on the element with $.style just for make the calcul conversion about the different CSS unities.
When I understand this, I understand the code BUT I didn't understand (again) why it's necessary to make the do{}while() loop, because every time this loop is passed only one time for each call.

gibson042 · 2015-01-13T20:54:34Z

Thanks for the followup! I think you will see some problems if you run the effects test suite against this code. Also, I think more thought needs to go into handling extreme values (zero, very small, very large)—which is the reason for do...while in effects.js.

And finally, this new functionality should come with unit tests so we can ensure its correct behavior on all platforms into the future.

mr21 · 2015-01-14T06:45:06Z

Hi @gibson042 :)

I forgot to check the division by zero, shame on me... I have update my commit.

I didn't understand what you mean by very small value it's between 0 and 1 pixel?
Like 0.5pt == 0.666px ?

It make sens for me only if we could multiply a value like when we multiply a float casted in int:
((int)2.5) * 100 == 200 instead of 2.5 * 100 == 250
I mean the tiny difference less than one pixel would increase with the multiplication. But we cannot multiply anyway.

gibson042 · 2015-01-14T13:55:47Z

"Very small" in the sense of "risking problems from rounding errors". The easiest way to get them is mixing "%" and non-"%", e.g. between width: 0.2% and 7em.

And again, all of this should have unit tests.

mr21 · 2015-01-15T00:17:23Z

Here I put several examples with the current jQuery
http://jsfiddle.net/Mr21/o4qbt6sf/

And here it's the same but with my changes (without the loop).
http://jsfiddle.net/Mr21/78yb124y/

The width of the blue bars are hard-fixed with the correct value and never move.
The width of the red and the green bars are also initialised with a specific values but I modify it to reach to the blue value:
But the red bars is modified with .css("width", "+=10px");
But the green bars is modified with .animate({width: "+=10px"}, 500);

I wrote your example (0.2% to 7em) at the end of list.
But I cannot find any example who show me the need of the do...while();

gibson042 · 2015-01-15T04:54:15Z

You're still dealing with smallish values, but dimensions can get arbitrarily large (remember, "%" is always relative)—the loop is a guarantee that we can establish an accurate scale even for extreme values.

All of which, of course, is completely beside the point while this PR is not adding unit tests for new functionality and is failing existing effects tests.

mr21 · 2015-01-15T13:20:57Z

I saw these failing tests, I will extract the loop in the common function this evening.
Is there a particular norme for adding some new unit tests?

gibson042 · 2015-01-15T14:53:40Z

Best judgment, and trying to preserve the surrounding style. In this case, I'd like to see a new test immediately after the existing relative-value test in test/unit/css.js that looks something like:

test( "css() non-px relative values (gh-1711)", <N>, function() {
    …
});

and covers various non-px scenarios.

mr21 · 2015-01-18T20:49:24Z

I made a new commit (all tests are passing).
But it was a little complicated to find something in common finally.
Tell me if you think it's okay.

And another thing: why not make an alias on:
.css(prop, value) or
.css({prop: value}) to call directly:
.animate({prop: value}, 0);
And on the animate side we would find a if to check if we launch or not a real animation.

I think it can be a great save a code, no?

mr21 · 2015-01-21T14:05:47Z

@gibson042 I add 10 new tests here; https://github.com/Mr21/jquery/blob/css/test/unit/css.js#L206-L245

arthurvr · 2015-01-21T14:13:28Z

src/css.js

@@ -285,7 +286,7 @@ jQuery.extend({

 			// If a number was passed in, add 'px' (except for certain CSS properties)
 			if ( type === "number" && !jQuery.cssNumber[ origName ] ) {
-				value += "px";
+				value += ret && ret[3] ? ret[3] : "px";


Nitpick but you should have spaces inside the square brackets.

Oh, thanks (it's done) :)

gibson042 · 2015-02-04T06:44:09Z

src/css/cssCastUnit.js

+	"../var/rfxnum"
+], function( jQuery, rfxnum ) {
+
+function cssCastUnit( elem, prop, parts, tween ) {


Variables here could have better names, especially now that this code is a fully-fledged function serving two purposes. I recommend something like:

cssCastUnit → adjustCSS

parts → value

getCSS → currentValue

target → initial

start → recasted

end → adjusted

gibson042 · 2015-02-04T06:59:41Z

👏 @mr21; this is looking great!

mr21 · 2015-02-04T13:19:32Z

All is done @gibson042 :)

I found a probleme with line-height, this property is set in the cssNumber object for Don't automatically add "px" to these possibly-unitless properties.
And if we not pass any unit to line-height by default it's em.
So I modified some code again here: https://github.com/jquery/jquery/pull/2011/files#diff-414c8f59bff0d1b63680b64763d8c529R288.

Thanks for all these indications!

gibson042 · 2015-02-04T18:58:07Z

I found a probleme with line-height, this property is set in the cssNumber object

Yep, that's exactly the kind of thing I was hoping to uncover.

Thanks for all these indications!

Thank you; I will try to look it over again today. Also, squashing everything into a single commit makes it hard to see changes over the course of this pull request... in the future, please add new commits and fast-forward instead of force-pushing replacements.

mr21 · 2015-02-04T19:23:10Z

Understood 👍

gibson042 · 2015-02-05T18:44:52Z

src/css.js

-], function( jQuery, pnum, access, rmargin, rnumnonpx, cssExpand, isHidden,
-	getStyles, curCSS, defaultDisplay, addGetHookIf, support, dataPriv ) {
+], function( jQuery, pnum, rfxnum, access, rmargin, rnumnonpx, cssExpand, isHidden,
+	getStyles, curCSS, adjustCSS, defaultDisplay, addGetHookIf, support, dataPriv ) {


Let's keep the "r" variables together (e.g., rfxnum before or after rnumnonpx).

gibson042 · 2015-02-05T19:19:31Z

src/css/adjustCSS.js

+		unit = value && value[ 3 ] || ( jQuery.cssNumber[ prop ] ? "" : "px" ),
+		// Starting value computation is required for potential unit mismatches
+		recasted = ( jQuery.cssNumber[ prop ] || unit !== "px" && +initial ) &&
+			rfxnum.exec( jQuery.css( elem, prop ) );


Seeing it all together, maybe a couple more renames are in order:

value → valueParts

recasted → initialInUnit

gibson042 · 2015-02-05T20:08:41Z

Almost there! Thanks again, @mr21.

mr21 · 2015-02-06T04:39:06Z

All is done (in a new commit this time).

gibson042 · 2015-02-08T20:58:04Z

Just one more thing before this lands (and sorry I didn't check earlier)... make sure to sign our CLA with the same email address you use for GitHub commits.

mr21 · 2015-02-08T21:11:02Z

Done :)

timmywil · 2015-02-08T21:17:06Z

Thanks @mr21! 👍

mr21 · 2015-02-20T10:21:07Z

Great ^^
So now, do I have to merge my two commits?

gibson042 · 2015-02-23T17:18:27Z

Nope; just leave this as-is and I expect to merge within the next seven days (and definitely before the 3.0 beta). Thanks again!

Fixes gh-1711 Closes gh-2011 (cherry picked from commit 9b03f6d) Conflicts: src/css.js src/effects.js

gibson042 · 2015-03-09T17:38:25Z

Thanks again, @mr21!

mr21 · 2015-03-09T18:03:11Z

You're welcome 😄

Fixes gh-1711 Closes gh-2011

mr21 force-pushed the css branch from 92ebc46 to 8ebd844 Compare January 14, 2015 06:23

mr21 force-pushed the css branch from 8ebd844 to 2470e0e Compare January 18, 2015 20:31

mr21 force-pushed the css branch from 2470e0e to 731f4dd Compare January 19, 2015 08:09

arthurvr reviewed Jan 21, 2015
View reviewed changes

mr21 force-pushed the css branch from 731f4dd to dea1e6d Compare January 21, 2015 14:56

gibson042 reviewed Feb 4, 2015
View reviewed changes

CSS: Effects: relative values

fbeaadd

mr21 force-pushed the css branch from dea1e6d to fbeaadd Compare February 4, 2015 13:10

gibson042 reviewed Feb 5, 2015
View reviewed changes

CSS: Effects: coding style

90394cf

gibson042 mentioned this pull request Feb 25, 2015

~2.1 .css() converts percents to pixels #2110

Closed

jquerybot added the CLA: Valid label Feb 26, 2015

gibson042 closed this in 9b03f6d Mar 9, 2015

gibson042 pushed a commit that referenced this pull request Mar 9, 2015

CSS: Support relative adjustment in any applicable unit

6fb2cef

Fixes gh-1711 Closes gh-2011 (cherry picked from commit 9b03f6d) Conflicts: src/css.js src/effects.js

markelog pushed a commit that referenced this pull request Nov 10, 2015

CSS: Support relative adjustment in any applicable unit

e8b4646

Fixes gh-1711 Closes gh-2011

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.css(), the values like +=10% aren't ignored anymore (like .animate()) #2011

.css(), the values like +=10% aren't ignored anymore (like .animate()) #2011

mr21 commented Jan 13, 2015

gibson042 commented Jan 13, 2015

mr21 commented Jan 14, 2015

gibson042 commented Jan 14, 2015

mr21 commented Jan 15, 2015

gibson042 commented Jan 15, 2015

mr21 commented Jan 15, 2015

gibson042 commented Jan 15, 2015

mr21 commented Jan 18, 2015

mr21 commented Jan 21, 2015

arthurvr Jan 21, 2015

mr21 Jan 21, 2015

gibson042 Feb 4, 2015

gibson042 commented Feb 4, 2015

mr21 commented Feb 4, 2015

gibson042 commented Feb 4, 2015

mr21 commented Feb 4, 2015

gibson042 Feb 5, 2015

gibson042 Feb 5, 2015

gibson042 commented Feb 5, 2015

mr21 commented Feb 6, 2015

gibson042 commented Feb 8, 2015

mr21 commented Feb 8, 2015

timmywil commented Feb 8, 2015

mr21 commented Feb 20, 2015

gibson042 commented Feb 23, 2015

gibson042 commented Mar 9, 2015

mr21 commented Mar 9, 2015

.css(), the values like +=10% aren't ignored anymore (like .animate()) #2011

.css(), the values like +=10% aren't ignored anymore (like .animate()) #2011

Conversation

mr21 commented Jan 13, 2015

gibson042 commented Jan 13, 2015

mr21 commented Jan 14, 2015

gibson042 commented Jan 14, 2015

mr21 commented Jan 15, 2015

gibson042 commented Jan 15, 2015

mr21 commented Jan 15, 2015

gibson042 commented Jan 15, 2015

mr21 commented Jan 18, 2015

mr21 commented Jan 21, 2015

arthurvr Jan 21, 2015

Choose a reason for hiding this comment

mr21 Jan 21, 2015

Choose a reason for hiding this comment

gibson042 Feb 4, 2015

Choose a reason for hiding this comment

gibson042 commented Feb 4, 2015

mr21 commented Feb 4, 2015

gibson042 commented Feb 4, 2015

mr21 commented Feb 4, 2015

gibson042 Feb 5, 2015

Choose a reason for hiding this comment

gibson042 Feb 5, 2015

Choose a reason for hiding this comment

gibson042 commented Feb 5, 2015

mr21 commented Feb 6, 2015

gibson042 commented Feb 8, 2015

mr21 commented Feb 8, 2015

timmywil commented Feb 8, 2015

mr21 commented Feb 20, 2015

gibson042 commented Feb 23, 2015

gibson042 commented Mar 9, 2015

mr21 commented Mar 9, 2015