search

cssinjs / css-vendor

Runtime vendor prefixing based on feature detection.
MIT License
66 stars 24 forks source link

Method supportedValue should support !important property. #214

Open LvChengbin opened 3 years ago

LvChengbin commented 3 years ago 
it( 'should support !important property (solid 1px red)', () => {
    expect( supportedValue( 'border', 'solid 1px red !important' ) ).to.be( 'solid 1px red !important' );
} );

it( 'should support !important property (1px solid red)', () => {
    expect( supportedValue( 'border', '1px solid red !important' ) ).to.be( '1px solid red !important' );
} );

I added these two test cases for supportedValue method, and got the output as follow:

✖ should support !important property (solid 1px red)
✔ should support !important property (1px solid red)
✖ should support !important property (solid 1px red)
        Chrome 86.0.4229.3 (Mac OS 10.15.6)
      Error: expected false to equal 'solid 1px red !important'
          at Assertion.assert (webpack://cssvendor/node_modules/expect.js/index.js:96:13 <- tests.webpack.js:8:10305)
          at Assertion.be.Assertion.equal (webpack://cssvendor/node_modules/expect.js/index.js:216:10 <- tests.webpack.js:8:11808)
          at Assertion.<computed> [as be] (webpack://cssvendor/node_modules/expect.js/index.js:69:24 <- tests.webpack.js:8:9695)
          at Context.<anonymous> (webpack://cssvendor/src/supported-value.test.js:83:81 <- tests.webpack.js:29:255519)

This issue makes using 1px solid red !important and solid 1px red !important has different result in jss. To support !important property or at least to provide same result with these two type of format.

I can send a pull request for this if you think it makes sense.

kof commented 3 years ago

Have you figured out why this happens?

LvChengbin commented 3 years ago

@kof

A values ends with !important cannot be added as a property of el.style directly, The !important should be removed before adding or just to use style.setProperty instead. https://github.com/cssinjs/css-vendor/blob/master/src/supported-value.js#L67

Using 1px solid red can pass check because the string starts with a number which matches the condition !isNaN(parseInt(prefixedValue, 10)) https://github.com/cssinjs/css-vendor/blob/master/src/supported-value.js#L52

kof commented 3 years ago

@LvChengbin @AleshaOleg I think nothing stops us from using el.style.setProperty instead of el.style.property! A PR with this would be appreciated along with this use case as a test

LvChengbin commented 3 years ago

I sent a pull request but failed to pass the test and got error message:

23 08 2020 16:23:19.652:INFO [launcher]: Launching browsers BS_MobileSafari, BS_Opera52, BS_Chrome, BS_Safari, BS_Firefox, BS_InternetExplorer9, BS_InternetExplorer10, BS_InternetExplorer11, BS_Edge13 with concurrency 2
23 08 2020 16:23:19.671:ERROR [launcher]: Cannot load browser "BS_MobileSafari"!
  Error: Password is required.
    at ApiClient.BaseClient (/home/travis/build/cssinjs/css-vendor/node_modules/browserstack/lib/client.js:21:9)
    at ApiClient.ApiBaseClient (/home/travis/build/cssinjs/css-vendor/node_modules/browserstack/lib/api.js:9:13)