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 Vivaldi support #81

Merged
merged 1 commit into from
Jul 2, 2016
Merged

Conversation

kmiyashiro
Copy link
Contributor

This adds support for Vivaldi, which is essentially Opera. https://en.wikipedia.org/wiki/Vivaldi_(web_browser)

@kmiyashiro
Copy link
Contributor Author

Looks like the code climate token is not set in travis, tests are passing though.

@javi7
Copy link

javi7 commented May 20, 2016

:shipit:

@robinweser
Copy link
Owner

Does Vivaldi's version always match Opera's version in terms of features? Else you would have problems.
If yes, I am happy to merge it, thanks :)

@kmiyashiro
Copy link
Contributor Author

@rofrischmann Vivaldi is only a few months old and is basically a fork of Opera (both of which use Blink). At this point, it's safe to say that it behaves exactly like Opera, and will continue to use the -webkit prefixes along with Blink.

@robinweser
Copy link
Owner

@kmiyashiro Thanks for the information. I actually know (and even use) Vivaldi, but did not research that. So I could say Vivaldi version 10 (theoretically) is exactly the same as Opera version 10? Because it actually depends on the display version value.

@kmiyashiro
Copy link
Contributor Author

The version numbers do not match, Vivaldi's release started at 1 and is currently on 1.1 I believe. The browser version comes out as NaN in getBrowserInformation, yet still prefixes all the properties correctly.

@kmiyashiro
Copy link
Contributor Author

@rofrischmann Do you have any insight as to why this change enables the correct prefixes, even with version mismatch with Opera? Is there any danger to merging this as-is, considering it fixes Vivaldi prefixes?

Today, inline-style-prefixer defaults prefix-all which tries to use legacy props for flexbox which totally breaks in Vivaldi.

@robinweser
Copy link
Owner

I guess as the version is NaN it uses 0 instead which is why it basically adds all prefixes common for Opera/Webkit. The prefix-all does not actually break within Vivaldi. You'd just need to resolve the fallback values as they're returned as an array (inline-style-prefixer will act the same soon).

If you're transforming to CSS you may just join those values to a string, if using inline (e.g. with React, right now the only solution is to check which value is supported - still they're going to ship a helper to deal with fallback values soon).
e.g.

{
   display: 'flex'
}

becomes

{ 
  display: ['-webkit-box', '-moz-box', '-ms-flexbox', '-webkit-flex', 'flex']
}

Anyways: I guess for now we're safe to merge this (as it adds support for Vivaldi) even it is uses the default version 0. It is still less prefixes than the prefix-all fallback produces.

@kmiyashiro
Copy link
Contributor Author

Thanks for the explanation on how the versioning works. For the prefix-all fallback, I'm not sure I follow. Are you saying I have to manually iterate through all the properties generated by inline-style-prefixer and then manually output multiple lines for that property?

{
  display: ['-webkit-box', '-moz-box'...]
}

to

{
  display: '-webkit-box',
  display: '-moz-box',
  ...
}

@robinweser
Copy link
Owner

Well it would be nice to be able to do this (in CSS e.g. you can also define the same property multiple times), but as its JavaScript how would you want to achieve that?

{
  display: '-webkit-box',
  display: '-moz-box',
}

actually outputs {display: '-moz-box'} as each property can only be used once. Depending on your use case you need to process those arrays. e.g. I'm working on a new API for JavaScript-based styles which get transformed to CSS. I am transforming those fallback values into: {display: '-webkit-box;display:-moz-box;display:-ms-flexbox;display:-webkit-flex;display:flex}. But if you're using e.g. React with inline styles you can't use those strings anymore => you somehow need to evaluate which value actually is supported and use that exact one.

I know this is kind of complicated and for sure not a proper solution (as it was before), but React is already working to get this done.

@kmiyashiro
Copy link
Contributor Author

Oh that's right, I'm getting my CSS/JS wires crossed, I meant to output the array as a style string. What I'm seeing in Vivaldi is that it's not translating display: flex at all, which breaks any flex layout. Are you saying I need an intermediate step to translate the display property manually for the prefix-all fallback?

Vivalid output:

flex: 2 1 0%;
flex-direction: column;
-webkit-box-orient: vertical;
-webkit-box-direction: normal;

@robinweser
Copy link
Owner

Oh well, so would you mind telling how you're using the prefixer?

If you're using CSS anyways you could just do sth. like

Object.keys(styles).forEach(property => {
  const value = styles[property]
  if (Array.isArray(value)) {
    styles[property] = value.join(';' + camelToDashCase(property) + ':')
  }
})

camelToDashCase: e.g. https://github.com/rofrischmann/fela/blob/master/modules/utils/camelToDashCase.js

@kmiyashiro
Copy link
Contributor Author

I'm using inline-style-prefixer to prefix an object and passing the result to React style. Kind of like this:

<div style={prefixer.prefix({
  display: 'flex',
  flex: 1,
  flexDirection: 'column'
})}>

@robinweser
Copy link
Owner

robinweser commented May 24, 2016

Ok I see. So you right now got a problem that is not actually solvable with pure inline styles. Check what I wrote here: mui/material-ui#4280 (comment), this might help to understand the problem. React team is already fixing it, so should can use it that way soon again.
For now: Either test with another browser which prefixes correctly or use a temporary CSS class for now e.g.

.display-flex {
  display: -webkit-box;
  display: -moz-box;
  ...
}

But I would rather just wait until they fixed it.

@kmiyashiro
Copy link
Contributor Author

Right, I'd rather just get this fixed for Vivaldi with webkit prefixes and wait for React to figure it out. Does that mean this can be merged? 😁

@robinweser robinweser merged commit 4822abd into robinweser:master Jul 2, 2016
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

3 participants