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

cross-browser innerhtml fixes #546

Merged
merged 2 commits into from
Dec 9, 2016
Merged

Conversation

Abazhenov
Copy link
Contributor

@Abazhenov Abazhenov commented Dec 9, 2016

Fixes some tests that were failing when running browsers other than google chrome.

Some tests were failing because where chrome would render: <input class="foo" id="test">
Firefox / safari / edge might render: <input id="test" class="foo">

This PR uses our innerHTML utility to fix those tests and ensure that the strings we are comparing rendered components to correspond to what they would be in the browser running the tests.

This only addresses compatibility issues that have to do with comparing HTML strings.

@trueadm
Copy link
Member

trueadm commented Dec 9, 2016

CCing @Havunen @nightwolfz @LukeSheard for review

@trueadm
Copy link
Member

trueadm commented Dec 9, 2016

Thanks @Abazhenov this is great stuff! :)

Have you tested that all tests pass on npm run test and npm run browser?

@trueadm
Copy link
Member

trueadm commented Dec 9, 2016

Meh, the Travis build failed due to an NVM issue..

@LukeSheard
Copy link
Contributor

I think you forgot to commit the tools file / import it from some of the tests. But otherwise this looks great!

@Abazhenov
Copy link
Contributor Author

All tests are passing on my end

@Abazhenov
Copy link
Contributor Author

Abazhenov commented Dec 9, 2016

@LukeSheard OK I'll look it over, innerHTML was already defined in src/tools/utils on line 11, also, some files already had it imported prior to my changes.

@Abazhenov
Copy link
Contributor Author

@LukeSheard hopefully this is more clear, innerHTML should be required in all the files that need it

@LukeSheard
Copy link
Contributor

Okay. I'm only on some app my iPhone so this isn't a proper review. Thanks!

@LukeSheard
Copy link
Contributor

I'm quite happy to merge this, but we should add a chore issue to tidy up all instances of testing HTML strings with ...innerHTML).to.equal('${STRING}') to use the innerHTML compare. I'll merge this and open an issue.

@LukeSheard LukeSheard merged commit 05710ef into infernojs:dev Dec 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants