Galooshi / happo

Visual diffing in CI for user interfaces
504 stars 16 forks source link

fix(firefox): handle data uris #209

Closed oliviertassinari closed 7 years ago

oliviertassinari commented 7 years ago

Closes #207.

Come into that issue trying to add happo to the storybook of react-instantsearch.

oliviertassinari commented 7 years ago

I have found a better solution closer to the root of the issue. I have noticed that Firefox escape the ' (where Chrome don't).

var url1 = window.getComputedStyle(document.querySelector(X)).getPropertyValue('background-image')

// Firefox: url1 = url("data:image/svg+xml;utf8,<svg%20viewBox=\'0%200%2010%209\'%20xmlns=\'http://www.w3.org/2000/svg\'><path%20d=\'M1%204.88l2.378%202.435L9.046%201.6\'%20stroke-width=\'1.6\'%20stroke=\'%23FFF\'%20fill=\'none\'%20fill-rule=\'evenodd\'%20stroke-linecap=\'round\'%20stroke-linejoin=\'round\'/></svg>")
// Chrome: url1 = url("data:image/svg+xml;utf8,<svg viewBox='0 0 10 9' xmlns='http://www.w3.org/2000/svg'><path d='M1 4.88l2.378 2.435L9.046 1.6' stroke-width='1.6' stroke='%23FFF' fill='none' fill-rule='evenodd' stroke-linecap='round' stroke-linejoin='round'/></svg>")
oliviertassinari commented 7 years ago

so let me know if you want any guidance there

Thanks for asking. I'm gonna try to integrate it with Travis CI, however, I'm planning on hijacking his original purpose to only takes the screenshots with Happo and to let Argos-CI handle the diffs and review process. The only issue I have seen so far with that integration is around the screenshot destination, original test names are base64 making the folder name harder to use. I'm wondering if you would be open adding a function option for generating the screenshots destination.

Finally, I feel like it would be really interesting to build a selenium grid target. We have been using vrtest on Material-UI to take the screenshots in a docker hosting a selenium grid, chrome & firefox, that works pretty well.

oliviertassinari commented 7 years ago

Now I have a reproduction example for that issue: https://github.com/algolia/react-instantsearch/pull/130. I have managed to make the 82 cases passed with the fix of the PR.

lencioni commented 7 years ago

I'm wondering if you would be open adding a function option for generating the screenshots destination.

I'm hesitant to open this up to be customized because I'm not sure how useful it would be for most people. However, we have been discussing changing how we approach uploading in #178. Do you think something like that would fit your use-case?

oliviertassinari commented 7 years ago

Thanks guys :).

I'm hesitant to open this up to be customized

@lencioni I can understand, I ended up writing a normalisation script. Without looking too much in details, I don't think that #178 would answer my issue.

trotzig commented 7 years ago

This was released in version 5.0.0-rc.6. Thanks @oliviertassinari!