Skip to content
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

Add setting CSS property with important priority #189

Closed

Conversation

@maximzasorin
Copy link

commented Apr 25, 2019

Hello, I added the ability to set properties with an important priority. No we can use it as follows:

$('#testFragment').css('paddingRight', '10px !important');

In the tests, I did not use getComputedStyle because of an error in jsdom, for which inline styles are always in priority, see: jsdom/jsdom#2485

#169

@coveralls

This comment has been minimized.

Copy link

commented Apr 25, 2019

Coverage Status

Coverage increased (+0.009%) to 95.763% when pulling 33836de on maximzasorin:feature/css-property-important into 13154e9 on webpro:master.

@webpro webpro closed this May 3, 2019

@maximzasorin

This comment has been minimized.

Copy link
Author

commented May 4, 2019

@webpro Please comment on PR.

@webpro

This comment has been minimized.

Copy link
Owner

commented May 4, 2019

@maximzasorin My sincere apologies, I've totally missed it! Greenkeeper was polluting this space with PRs. Looking into it right now.

@webpro webpro reopened this May 4, 2019

@webpro
Copy link
Owner

left a comment

I like that we're using getPropertyValue now 👍

if(styleProps[prop] || styleProps[prop] === 0) {
element.style[prop] = styleProps[prop];
element.style.setProperty(prop, styleProps[prop], important);

This comment has been minimized.

Copy link
@webpro

webpro May 4, 2019

Owner

Let's optimize by doing something like this (not tested):

const important = /!important$/.test(styleProps[prop]) ? 'important' : undefined;
element.style.setProperty(prop, styleProps[prop].replace(/!important$/, ''), important);

Then the logic above it isn't executed if we're removing a property.

This comment has been minimized.

Copy link
@maximzasorin

maximzasorin May 4, 2019

Author

Thanks, I'll fix it.

@webpro

This comment has been minimized.

Copy link
Owner

commented May 4, 2019

Btw, why don't we use this API:

$('#testFragment').css('paddingRight', '10px', 'important');

Makes implementation faster and simpler. How does jQuery do this?

@maximzasorin

This comment has been minimized.

Copy link
Author

commented May 4, 2019

@webpro

Btw, why don't we use this API:

$('#testFragment').css('paddingRight', '10px', 'important');

What will the API look like when setting multiple properties?

How does jQuery do this?

As far as I know jQuery does not support important properties via css method. From docs:

Note: .css() ignores !important declarations. So, the statement $( "p" ).css( "color", "red !important" ) does not turn the color of all paragraphs in the page to red. It's strongly advised to use classes instead; otherwise use a jQuery plugin.

But they have an issue: jquery/jquery#3713

@maximzasorin

This comment has been minimized.

Copy link
Author

commented May 4, 2019

@webpro Added fix.

src/css.js Outdated
const important = /!important$/.test(styleProps[prop]) ? 'important' : undefined;
if(typeof styleProps[prop] === 'string') {
styleProps[prop] = styleProps[prop].replace(/\s*!important$/, '');
}

This comment has been minimized.

Copy link
@webpro

webpro May 4, 2019

Owner

Sorry for being picky, how about this (one line and slightly better readable?):

styleProps[prop] = important ? styleProps[prop].replace(/\s*!important$/, '') : styleProps[prop];

This comment has been minimized.

Copy link
@maximzasorin
assert(actualValue[0] === expected[0]);
assert(actualPriority[0] === 'important');
assert(actualValue[1] === expected[1]);
assert(actualPriority[1] === '');

This comment has been minimized.

Copy link
@webpro

webpro May 4, 2019

Owner

Thanks for adding the tests, super useful! I know I have this actual/expected style everywhere, but could you please consider this for conciseness here:

assert(element[0].style.getPropertyValue('font-weight') === expected[0]);
assert(element[0].style.getPropertyPriority('font-weight') === 'important');
assert(element[0].style.getPropertyValue('font-style') === expected[1]);
assert(element[0].style.getPropertyPriority('font-style') === '');

This comment has been minimized.

Copy link
@maximzasorin
var element = $('#testFragment')[0];
var actualValue = element.style.getPropertyValue('padding-right');
var actualPriority = element.style.getPropertyPriority('padding-right');
console.log('actual/expected', actualValue, expected);

This comment has been minimized.

Copy link
@webpro

webpro May 4, 2019

Owner

Please remove this line

This comment has been minimized.

Copy link
@maximzasorin

maximzasorin May 6, 2019

Author

Removed.

@webpro

This comment has been minimized.

Copy link
Owner

commented May 4, 2019

When sending the tests to BrowserStack, the CSS tests fail in IE and Edge:

  • Windows 8, Internet Explorer 10.0
  • Windows 7, Internet Explorer 11.0
  • Windows 10, Edge 17.0
  • Windows 10, Edge 18.0

Without any useful error/trace (Unspecified AssertionError). Do you happen to have one of these ready to test/debug?

@maximzasorin

This comment has been minimized.

Copy link
Author

commented May 6, 2019

@webpro

When sending the tests to BrowserStack, the CSS tests fail in IE and Edge:

Windows 8, Internet Explorer 10.0
Windows 7, Internet Explorer 11.0
Windows 10, Edge 17.0
Windows 10, Edge 18.0
Without any useful error/trace (Unspecified AssertionError). Do you happen to have one of these ready to test/debug?

I have none of this, but I debugged it in Edge on BrowserStack and found an error. It looks like Edge otherwise processes the value undefined for the priority argument. Edge accepts only the empty string or "important", otherwise, the value of the property does not change. And this behavior is very similar to what is described in the specification and differs from MDN:

  1. If priority is not the empty string and is not an ASCII case-insensitive match for the string "important", then return.

So I replaced the default value for the important argument from undefined to an empty string. Now the tests are correctly executed in Edge, if you have the opportunity to test in IE, then please do it.

@maximzasorin

This comment has been minimized.

Copy link
Author

commented May 11, 2019

@webpro

I tested the code in IE 11 and found a problem in the work of the setProperty method.

If we already have a property with priority = important:

element[0].style.setProperty('font-size', '15px', 'important')

And then try to set the same property without priority:

element[0].style.setProperty('font-size', '20px', '')

That in Chrome, Firefox, Safari and Edge we get the value=20px and priority='', but in IE we get the value=15px and priority='important'. It turns out that the setProperty method in IE does not overwrite the value of the property that is set to important priority.

The only way out I see is to remove the property before setting a new value, something like is:

if (!important) {
    element[0].style.removeProperty('font-size')
}
element[0].style.setProperty('font-size', '20px', '')

What do you think about it?

@webpro

This comment has been minimized.

Copy link
Owner

commented May 29, 2019

Apologies for my late response. First of all, thanks a lot for putting in all the efforts and test it in IE11! Overall, by now I think the required changes/maintenance to support a bad practice (usage of !important in CSS) do not outweigh the advantages. It's also better practice to use and (un)set classes (over .css() anyway, so therefore I'm going to close this issue. I hope you agree with me. Again, thanks for your time and interest in DOMtastic!

@webpro webpro closed this May 29, 2019

@maximzasorin

This comment has been minimized.

Copy link
Author

commented May 29, 2019

Thanks for the comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.