search

cssinjs / css-vendor

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

Add support for "hyphens" CSS property #194

Open fcaylus opened 4 years ago

fcaylus commented 4 years ago

As specified here: https://caniuse.com/#search=hyphens, it should be prefixed on IE and Safari

AleshaOleg commented 4 years ago

thanks for your PR @fcaylus. As we're not detecting browser version can you also please prefixes for Firefox and Edge? What we're usually doing in this case, just prefixing property for any version. It will anyway work in the newest version of these browsers, but with prefix old versions (Firefox from 6 till 42 and Edge from 12 to 18) also will be supported. Here are examples:

Firefox https://jsfiddle.net/Lg8wp4xf/ Edge - https://jsfiddle.net/Lg8wp4xf/1/

Can you please make changes? Thanks!

fcaylus commented 4 years ago

Hello @AleshaOleg . Of course, I will change it. I actually wasn't sure about how I should handle this problem, so I prefered not prefixing anything.

I'll do it tomorrow.

fcaylus commented 4 years ago

I was a bit busy last week but I finally made the modifications for Edge and Firefox.

AleshaOleg commented 4 years ago

So, I just ran tests locally for this PR. And it only fails for Mozilla, because we're using version 75 for our tests. And this version doesn't require a prefix. What we're usually doing in such cases, simply ignoring this test, like here - https://github.com/cssinjs/css-vendor/blob/master/tests/fixtures.js#L51

Also, here you added scopes - https://github.com/cssinjs/css-vendor/pull/194/files#diff-910474ccfd83e0d91c039c059135004fR55. Can you please remove them, there's no lint rule for this, I thought we have. But I'm trying to write such cases without scopes.

Otherwise, no more comments from me. And after those issues will be fixed at least first one, I can't force you to fix seconds because there's no lint rule for this.

After, I'll run tests again and will be glad to merge PR.

Thanks for your effort @fcaylus :)