Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(jqLite): .css() retrieves computed style also #8161

Closed
wants to merge 1 commit into from
Closed

fix(jqLite): .css() retrieves computed style also #8161

wants to merge 1 commit into from

Conversation

sagens42
Copy link
Contributor

fixes #2866

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#8161)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@sagens42 sagens42 added cla: yes and removed cla: no labels Jul 12, 2014
@@ -825,6 +825,17 @@ describe('jqLite', function() {
}
});

it('should get computed styles also', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove "also"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@btford sure

@sagens42
Copy link
Contributor Author

It seems it fails for IE versions. I will look on it tomorrow

@sagens42
Copy link
Contributor Author

Now everything is good. Can you review, please, @btford

jqLite(document).find('body').append(jqA);

var style = document.createElement('style');
style.appendChild(document.createTextNode('#elementA { width: 25px; }'));
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to ensure that this style gets removed via finally or afterEach

@IgorMinar
Copy link
Contributor

@matsko I thought more about this PR. Do we need this feature in ngAnimate? the general policy for jqLite is that unless it's a feature required by Angular we shouldn't add it to jqLite.

how is this different from large number of features supported by jQuery but by jqLite?

@matsko
Copy link
Contributor

matsko commented Jul 16, 2014

@sagens42 my apologies, but we can't put this feature into jqLite. The reasoning being is that the jQuery implementation has a large amount of code to make this work and it's a feature in Angular that isn't worth maintaining (special casing for IE and an internal caching system for repeated calls to getComputedStyle). The other issue is that for any other Angular applications that do not use jQuery and rely on .css then they could see a major performance hit from repeated calls to getComputedStyle. While the idea here is great, it isn't worth the risk of having to potentially patch it down the road.

Also, with 2.0, jqLite won't be around anyway. So we're trying to avoid extending it. I will add a commit that explains in the docs exactly why .css doesn't compute it for you.

Thanks a bunch for putting the PR together.

@matsko matsko closed this Jul 16, 2014
@sagens42 sagens42 deleted the fix/jqLiteCss branch July 21, 2014 09:59
@sagens42 sagens42 restored the fix/jqLiteCss branch July 21, 2014 10:00
@gkalpak
Copy link
Member

gkalpak commented Sep 19, 2014

@matsko: Can I find more info on what will happen to jqLite in v2.0 somewhere ?
I have seen some design docs, but I can't find anything about jqLite being removed (maybe I missed).
(Will we be forced to use jQuery or what ?)

@matsko
Copy link
Contributor

matsko commented Sep 23, 2014

@gkalpak from what I know after chatting with @IgorMinar is that in Angular 2.0 jqLite may not be around and we may end up sticking to vanilla ES6 DOM stuff.

@gkalpak
Copy link
Member

gkalpak commented Sep 23, 2014

@matsko, thx for the response. vanilla DOM stuff sounds good :)

@ninjasort
Copy link

Interesting.. I like the idea that jqLite might be removed. It would definitely save headaches around the absence of jQuery idiosyncrasies.

But would that mean we would include jQuery? .css(propertyName) and .hasClass() seem really great for testing directives.

@caitp
Copy link
Contributor

caitp commented Oct 1, 2014

nobody really knows for sure yet

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jqLite .css() retrieves only inline styles
9 participants