-
Notifications
You must be signed in to change notification settings - Fork 90
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
feat: Add video tag support for context #87
feat: Add video tag support for context #87
Conversation
@@ -7,6 +7,7 @@ import styles from './test.css'; | |||
|
|||
const cx = classNames.bind(styles); | |||
|
|||
const videoRegEx = /(?:mp4|webm)$/i; |
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.
More extensions could be added here. These are common and non-ambiguous.
|
||
return ( | ||
<video controls src={ linkUrl } className={ cx('video') }> | ||
<track kind='captions' /> |
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.
The accessibility checker flags video tags without captions even though there is caption source file. I didn't know what would be better - disabling the lint rule or fooling it. Either way there are no captions.
src/components/test/test.css
Outdated
@@ -264,3 +264,9 @@ | |||
max-width: 100%; | |||
height: auto; | |||
} | |||
|
|||
.video { | |||
display: block; |
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 as img CSS. Makes the video scale responsively like the image tag does.
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 should be combined with the .image
class definition since they are the same.
The video support is simple and automatic. I don't know if the maintainer wants a more formal context type to opt in to more video features (captions, poster, multiple sources, etc). This change works well for adding video as context for failures of web UI tests (Cypress or Selenium) |
@NicholasBoll Thanks for the PR! This looks good overall, just a comment about the CSS. Also, could you add tests for this? |
Sure thing. Thanks for taking a look! |
}); | ||
expect(wrapper).to.have.className('test'); | ||
expect(snippet).to.have.lengthOf(0); | ||
expect(wrapper).to.have.descendants('video'); |
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 did expect(wrapper).to.have.descendants('video')
instead of expect(video).to.have.lengthOf(1)
for the failure case.
expect(video).to.have.lengthOf(1);
// failure: Expected 0 to be 1
expect(wrapper).to.have.descendants('video');
// failure: expected <TestContext /> to have descendants 'video' (and then dumps the HTML for easy comparison)
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 like this change but it looks like we could keep the length check using:
expect(wrapper).to.have.exactly(1).descendants('video');
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 like it
@adamgruber I've fixed the CSS, added an anchor tag in the video tag (in case the browser doesn't render the video tag - the anchor will render instead). I also added tests, but I changed the style slightly for better failure cases. |
expect(wrapper).to.have.descendants('video'); | ||
expect(wrapper).to.have.descendants('a.test-video-link'); | ||
expect(videoLink).to.have.attr('href', 'http://test.url.com/testvideo.mp4'); | ||
videoLink.simulate('click', { stopPropagation: noop }); |
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.
What's the reason for the simulated click here?
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 don't know, it was part of the img one I copied.
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.
Haha. Ok fair enough. I don't remember why I added that.
@adamgruber I've made the suggested changes. Thank you for taking the time to make this PR better |
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.
Nice work, thanks for this.
This is a very simple implementation of adding video support. It recognizes a video link in the context and makes a
video
tag instead of just a URL. It follows the same implementation of theimg
tag. Not all video extensions are supported - just the ones I tested. More could easily be added.It looks like this: