Add setting CSS property with important priority by maximzasorin · Pull Request #189 · webpro/DOMtastic

maximzasorin · 2019-04-25T21:37:17Z

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 · 2019-04-25T21:42:54Z

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

maximzasorin · 2019-05-04T13:09:09Z

@webpro Please comment on PR.

webpro · 2019-05-04T13:16:24Z

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

webpro

I like that we're using getPropertyValue now 👍

webpro · 2019-05-04T13:22:16Z

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


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.

Thanks, I'll fix it.

webpro · 2019-05-04T13:30:16Z

Btw, why don't we use this API:

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

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

maximzasorin · 2019-05-04T13:49:29Z

@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 · 2019-05-04T14:17:59Z

@webpro Added fix.

webpro · 2019-05-04T19:30:32Z

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


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

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

webpro · 2019-05-04T19:32:30Z

+      assert(actualValue[0] === expected[0]);
+      assert(actualPriority[0] === 'important');
+      assert(actualValue[1] === expected[1]);
+      assert(actualPriority[1] === '');


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') === '');

webpro · 2019-05-04T19:32:44Z

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


Please remove this line

webpro · 2019-05-04T19:42:20Z

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 · 2019-05-06T22:16:22Z

@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:

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 · 2019-05-11T09:59:31Z

@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 · 2019-05-29T19:13:31Z

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!

maximzasorin · 2019-05-29T21:37:18Z

Thanks for the comment.

maximzasorin added 2 commits April 26, 2019 00:27

Add setting css property with important priority

3aecc24

Add tests for setting css property with important priority

19f883d

webpro closed this May 3, 2019

webpro reopened this May 4, 2019

webpro suggested changes May 4, 2019

View reviewed changes

Avoid check important css property for removing value

8101ee9

webpro suggested changes May 4, 2019

View reviewed changes

maximzasorin added 2 commits May 7, 2019 00:59

Fix tests for setting css property with important priority

7031366

Fix setting css priority in Edge

33836de

webpro closed this May 29, 2019

Conversation

maximzasorin commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maximzasorin commented May 4, 2019

Uh oh!

webpro commented May 4, 2019

Uh oh!

webpro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webpro commented May 4, 2019

Uh oh!

maximzasorin commented May 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maximzasorin commented May 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webpro commented May 4, 2019

Uh oh!

maximzasorin commented May 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maximzasorin commented May 11, 2019

Uh oh!

webpro commented May 29, 2019

Uh oh!

maximzasorin commented May 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

maximzasorin commented Apr 25, 2019 •

edited

Loading

coveralls commented Apr 25, 2019 •

edited

Loading

maximzasorin commented May 4, 2019 •

edited

Loading

maximzasorin commented May 6, 2019 •

edited

Loading