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

ensure modernizr can run inside an iframe #1581

Merged

Conversation

patrickkettner
Copy link
Member

fixes #887

@patrickkettner patrickkettner changed the title updates and saucelabs desktop support ensure modernizr can run inside an iframe Apr 19, 2015
@patrickkettner
Copy link
Member Author

so none of the code from #887 exists any longer, and from my local testing of this change there is no longer any issue actually running the full build inside of an iframe.

@mikkotikkanen can you confirm that all that was needed to ensure its not broken is loading and parsing the script inside of the context of the iframe?

cc @ryanseddon @stucox

@modernizr-savage
Copy link

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 2c98c24
Build details: https://travis-ci.org/modernizr-savage/Modernizr/builds/59092585

(Please note that this is a fully automated comment.)

@mikkotikkanen
Copy link
Contributor

Not really working on that project anymore, so no idea (my pull request is over 2 years old :P ). The test seems OK, though it's missing the case of running the Modernizr inside a scope.

Something along the lines of...

(function() {
  <insert Modernizr here>
})();

...so that this doesn't point to window.

@patrickkettner
Copy link
Member Author

we are not using this anymore, so we should be golden.

patrickkettner added a commit that referenced this pull request Apr 21, 2015
ensure modernizr can run inside an iframe
@patrickkettner patrickkettner merged commit 9e249d5 into Modernizr:master Apr 21, 2015
@patrickkettner patrickkettner deleted the confirm-iframe-parsing branch April 21, 2015 19:54
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