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 support for "hyphens" CSS property #194

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add support for "hyphens" CSS property #194

wants to merge 2 commits into from

Conversation

fcaylus
Copy link

@fcaylus fcaylus commented Apr 8, 2020

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

Signed-off-by: Fabien Caylus <fabien.caylus@gmail.com>
@AleshaOleg
Copy link
Member

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
Copy link
Author

fcaylus commented Apr 14, 2020

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
Copy link
Author

fcaylus commented Apr 18, 2020

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

@AleshaOleg
Copy link
Member

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

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

Successfully merging this pull request may close these issues.

None yet

2 participants