Skip to content

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

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.

Sign up for GitHub

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
Closed
mr21 wants to merge 0 commits into from
Closed

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

mr21 wants to merge 0 commits into from

Conversation

@mr21
Copy link
Contributor

@mr21 mr21 commented Jan 22, 2014

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
Copy link
Member

gibson042 commented Jan 27, 2014

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
Copy link
Contributor Author

mr21 commented Jan 27, 2014

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

@dmethvin
Copy link
Member

dmethvin commented Mar 14, 2014

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

@mr21
Copy link
Contributor Author

mr21 commented Mar 15, 2014

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
Copy link
Contributor Author

mr21 commented Mar 15, 2014

@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 Mar 18, 2014
@dmethvin
Copy link
Member

dmethvin commented Mar 18, 2014

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
Copy link
Contributor Author

mr21 commented Mar 18, 2014

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
Copy link
Member

dmethvin commented Mar 19, 2014

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

@mr21
Copy link
Contributor Author

mr21 commented Apr 13, 2014

@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
Copy link
Contributor Author

mr21 commented Apr 25, 2014

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

@dmethvin
Copy link
Member

dmethvin commented Apr 29, 2014

@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
Copy link
Contributor Author

mr21 commented May 15, 2014

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
Copy link
Member

dmethvin commented May 15, 2014

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
Copy link
Contributor Author

mr21 commented May 16, 2014

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
Copy link
Member

gibson042 commented May 19, 2014

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 Dec 10, 2014
@mr21 mr21 closed this Jan 13, 2015
@mr21 mr21 mentioned this pull request Jan 13, 2015
Closed
@mgol mgol removed this from the 3.0.0 milestone Oct 17, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

CSS

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mr21 @gibson042 @dmethvin @timmywil @mgol