-
Notifications
You must be signed in to change notification settings - Fork 28
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
turbolinks accepts a document, run tests inside an iframe #163
Conversation
Nice :D |
afterEach -> | ||
Turbolinks.document(testDocument) | ||
|
||
it 'assigning a document', -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to state some kind of behaviour, or cause and effect. Or just delete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll delete it :P
I can run browser tests! 😃 Excellent stuff 👍 |
1f59b1c
to
34a7bb6
Compare
…an iframe group iframe setup into function remove silly test, fix results
34a7bb6
to
9eee511
Compare
@qq99 You think this is good to 🚢 ? |
@@ -42,7 +43,7 @@ requestMethodIsSafe = | |||
browserSupportsTurbolinks = browserSupportsPushState and requestMethodIsSafe | |||
|
|||
browserSupportsCustomEvents = | |||
document.addEventListener and document.createEvent | |||
activeDocument.addEventListener and activeDocument.createEvent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could probably just be document
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well you actually want to know it about the document you're operating on, but it usually shouldn't matter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM. Looks very no-op-y for consumers
I did not review the tests however but if they run in the browser now, that's fantastic!
We now have a browser runner again!
(closes #138)
This PR makes two primary changes.
Turbolinks.document
allows you to set a document for turbolinks to operate on.How
We had to move the way we defined ASSET_FIXTURES due to some weirdness in teaspoon config, and change all uses of
document
to reference a variable in module scope to facilitate this change. We also had to make some minor changes in how we initialize tests.Why
Development velocity is better when we can actually use the browser test runner. Everything else is gravy.
Follow up:
We should change
remote_test
to do this as well, since it's currently suffering from the same problemturbolinks_test
had.Reviewers
@qq99
@GoodForOneFare
@edward
CC @Shopify/tnt