-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Implement link validation #571
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
Conversation
parisk
left a comment
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.
Looks great. I am also fond of ditching Phantom in favor of jsdom.
Just a couple of comments on file naming.
test-harness.html
Outdated
| assert = chai.assert | ||
| </script> | ||
| <script src="build/xterm.js"></script> | ||
| <script src="lib/Linkifier.phantom.js"></script> |
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.
Should we change the naming from *.phantom.js to *.jsdom.js?
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.
Opps, the test harness was meant to be deleted. Fixed in next commit. jsdom tests are now run with the regular tests so no need for a custom name.
test-harness.html
Outdated
| </script> | ||
| <script src="build/xterm.js"></script> | ||
| <script src="lib/Linkifier.phantom.js"></script> | ||
| <script src="lib/utils/CharMeasure.phantom.js"></script> |
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.
Same here, should we change the naming from *.phantom.js to *.jsdom.js since we ditched PhantomJS in favor of jsdom?
Fixes #570
This allows consumers to invalidate links:
As part of this I also removed PhantomJS in favor of much more lightweight jsdom. This makes the tests far more readable, is way faster to download and run and everything runs through the mocha gulp task so they're included in the coverage report!
The one downside with jsdom is that it doesn't do layouts so we have to fake them in the tests, this is a small price to pay for the benefits though imo.