cssinjs / css-vendor

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

Fix failing tests 4 #18

Closed cvle closed 7 years ago

cvle commented 7 years ago

With this PR our CI passes except for flex and IE + Edge issues.

Related PRs:

kof commented 7 years ago

nice

cvle commented 7 years ago

I actually started implementing flexbox spec from 2009 but we cannot do it with the current API

kof commented 7 years ago

I actually started implementing flexbox spec from 2009 but we cannot do it with the current API

Maybe we also don't need to, we should see the browser usage for that old spec.

kof commented 7 years ago

merged

cvle commented 7 years ago

I looked through the statistics. Flexbox spec 2009 is used in 0,9% of browsers excluding the UC browser. Including the UC browser it's almost 10% (mostly india, china and indonesia).

kof commented 7 years ago

UC?

cvle commented 7 years ago

Though UC Browser is a pain in the ass to feature detect. I'd say lets proceed with current API and implement flexbox spec 2012 for IE & Edge.

cvle commented 7 years ago

It's a browser from china that is gaining world wide popularity but especially in emerging markets like India, China and Indonesia

kof commented 7 years ago

Wow, never heard of it. But yeah lets skip it for now.

kof commented 7 years ago

Does autoprefixer support that spec?

cvle commented 7 years ago

Yes and inline-style-prefixer too. The issue with our API is that to support the old spec we need to split flew-flow into two longhand props. We can't do that with our current API.

Though to really support the UC Browser a few hacks will probably necessary because they messed up stuff and made it hard to feature detect.

I will look into whether the 2012 spec for IE is compatible with our current API.

kof commented 7 years ago

Maybe we don't need to feature detect that stuff after all, but just resort for static map for the flexbox.

cvle commented 7 years ago

feature detection is easy, only the UC Browser is a bit tricky e.g. it accepts a flex-wrap property even though it does not support it xD but I believe there is a way to detect the UC browser too.

kof commented 7 years ago

feature detection is easy, only the UC Browser is a bit tricky e.g. it accepts a flex-wrap property even though it does not support i

Thats why feature detection can't be used there…

kof commented 7 years ago

I mean in a useful way

kof commented 7 years ago

so maybe we can have a static map for this case, if a prop is not there, we start feature detection.

cvle commented 7 years ago

As long as it does not affect others browsers, sure.

cvle commented 7 years ago

I still hope that at one point they will update to a newer Chromium version and then this would be a non issue. At least it is an evergreen browser.

kof commented 7 years ago

Yep, for this reason we should keep this logic in a separate file we can simply remove when this happens.

cvle commented 7 years ago

Are you in favor of changing the API to support the old flexbox spec?

kof commented 7 years ago

I am not sure what the changes are, but they are internal, right?

cvle commented 7 years ago

So currently we have this:

cssVendor.supportedProperty('animation')  // -webkit-animation
cssVendor.supportedValue('display', 'flex') // -webkit-flex

But we would need this

cssVendor.supported('flex-flow', 'row-reverse')  
{
  -webkit-box-orient: 'horizontal',
  -webkit-box-direction: 'reverse',
}
kof commented 7 years ago

https://developer.mozilla.org/en-US/docs/Web/API/CSS/supports

it returns just a boolean though.

kof commented 7 years ago

So in your example it returns an object, are you expecting that the user applies all returned properties/values?

cvle commented 7 years ago

Exactly, both need to be set in order to support the old spec. That's what came in my mind first. What are your thoughts?

cvle commented 7 years ago

Though it was more of an example to show you what is the problematic than an actual well-thought solution :-)

cvle commented 7 years ago

This would be closer to our current architecture, easier to implement and probably faster:

const prop = cssVendor.supportedProperty('flex-flow')  // ['-webkit-box-orient', '-webkit-box-direction']
if (Array.isArray(prop)) {
  prop.forEach(
    p => cssVendor.supportedValue(p, 'row-reverse'),
  )
} 
else {
  cssVendor.supportedValue(prop, 'row-reverse')
}
kof commented 7 years ago

Yeah this one looks a bit better, though its a breaking change. What if we introduce a supportedProperties(prop1, prop2, …) function which expect a number of properties and returns an array always. This method will be used by those who cares about legacy browsers and at some point it can be removed.

This way we keep supportedProperty interface as it was before, you pass a prop, you receive a prop. Same for supportedProperties, you pass a number of props, you receive a number of props.

Also it could be supportedProperties([prop1, prop2, …])

cvle commented 7 years ago

There is an issue with your proposal because when I have this supportedProperties('height', 'flex-flow', 'width') and I get ['height', '-webkit-box-orient', '-webkit-box-direction', 'width'], I wouldn't know how the new properties relate to the one I passed.

We could use supportedProperties(prop) that only accepts one prop and always returns an array and we keep supportedProperty for backwards compatibility.

kof commented 7 years ago

True. Another idea supportedProperty(prop, supportMultiple). This way we don't have to return always an array and it is explicit. My previous suggestion would lead to actually generating arrays always, even if there is no need. Now if supportMultiple is true, the return might be an array if needed.

cvle commented 7 years ago

That's good. Do you agree with the following?

supportedProperty(prop, {multiple: true})
kof commented 7 years ago

yeah, that is even better for the future

kof commented 7 years ago

by default multiple should be false