Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

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

Closed
wants to merge 0 commits into from

4 participants

@Mr21

Hi :)

Actually when we make:

$this.animate({width: "+=10%"}, 500);

it's work fine, the value is converted into percentage BUT if we write:

$this.css("width", "+=10%");

We don't have the same behavior...
The .css() function doesn't look the unity, it's "px" every time.

@gibson042
Collaborator

This is specific to "%", but we want the functionality to work in all units, essentially moving some logic from effects to css. Would you like to try your hand at the general solution?

@Mr21

Yes! I'll try very soon.
Thanks :)

@dmethvin
Owner

@Mr21 I'd love to land this soon. Do you think you'll have a chance to finish it?

@Mr21

Hi,
I'm really sorry, I didn't find the time to finish this but I continue actively since yesterday.
I will push to night.

@Mr21

@dmethvin
To allow the possibility of using the operator += with any CSS units I wrote the function .pixelsToUnity().
And to retrieve the size of a mm/cm/inch/point/etc. I had to create a <div> and bind it to the <body> to calculate its size in pixels.
Even if I do that only one time (at the first call of pixelsToUnity), I'm not sure if this is correct.
But I don't know how to do otherwise, I'm sure I read the code but I didn't find how the .animate() function works.

@dmethvin dmethvin added this to the 1.12/2.2 milestone
@dmethvin
Owner

I don't think that will work, especially once it is back-ported into the 1.x-master branch as it must be. Try your patch there and run unit tests to see what I mean. This is a lot of code and will get bigger once it has enough protection around the CSS values like "auto". Plus, solutions that inject content into the body tend to create problems of their own. I'll put this in the 1.12/2.2 milestone since it would definitely not be something for a patch release.

@Mr21

Hi,
How can we know the DPI of the user's screen if we can't add something?
I searched but I didn't find anything (except this technique).
I think if there was a standard value it will be 96 but I know we can't hardcode this value obviously.
I remove the <div> immediately after I took what I had to take (with offsetWidth).
Do you have another idea?

About the auto value, I don't understand because the function .css() convert that already. It's the same about inherit I saw.

(For now, the code don't change anything for all the scripts already written, because they use only +=1px or +=1.)

@dmethvin
Owner

Try this patch in the 1.x-master branch with IE8, for example.

@Mr21

@dmethvin hi,

Sorry for the delay...

I made another commit who secure the auto CSS value for the $.css() method.
With this page Mozilla - auto I think there is only the margin property who need to be computed.

But this problem existed since the begining.
Before my PR, if you had made $elem.css("margin-left", "+=2"); on an element who has margin:auto; it would not have worked either (on IE8)..

@Mr21

Should I create another PR on the 1.x branch for my last commit about IE8?

@dmethvin
Owner

@Mr21 This is slotted for the 1.12/2.2 release so it will be a few weeks before we land it. In the meantime if you'd like to create a 1.x patch that would be fine, although it may need to be rebased by that time.

@Mr21

Hi @dmethvin,
I merged all commits who are related to the += feature.
I've rewrited the second commit here again, because I've found that IE9 and Firefox don't find the correct value for the margin property.
So I'm not sure if this commit has to be in the 1.x branch.

@dmethvin
Owner

I think it would need to be in both branches since it affects browsers supported in both. My main concern at this point is the size of the patch.

@Mr21

I reduce the code for the margin patch, what do you think?

In resume:

  • Firefox, IE9 : .css( "margin" ) return an empty string "".
  • IE<9 : The auto value is not computed.
  • Firefox : The auto value is computed BUT it's converted with 0px (cf: bug).

I patch the two first bugs, the last one it's not possible I think... How can we know if 0px it's for auto value or it's a real 0px value? :/

@gibson042
Collaborator

This is still enormous. @Mr21, here is the effects code accomplishing the desired results that I think should be moved into css (possibly with some changes) for resolution: https://github.com/Mr21/jquery/blob/css/src/effects.js#L26-L58

Do you think you could handle such an effort?

@timmywil timmywil added the CSS label
@Mr21 Mr21 closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Mr21:css was force-pushed and no longer has any new commits.

Pushing new commits will allow the pull request to be re-opened.

This page is out of date. Refresh to see the latest.

Mr21:css was force-pushed and no longer has any new commits.

Pushing new commits will allow the pull request to be re-opened.

Something went wrong with that request. Please try again.