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.

Sign up for GitHub

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

Jump to bottom

feat: Add video tag support for context #87

Merged
merged 4 commits into from
Jan 23, 2018
Merged

feat: Add video tag support for context #87

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a0d40ff
feat: Add video tag support for context
NicholasBoll Jan 20, 2018
94b7ff9
Add tests and merge CSS rules
NicholasBoll Jan 23, 2018
f4104b4
Fix lint error
NicholasBoll Jan 23, 2018
ed99dc3
Added length check to tests
NicholasBoll Jan 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/components/test/context.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,14 @@ class TestContext extends Component {
return (
<video controls src={ linkUrl } className={ cx('video') }>
<track kind='captions' />
Copy link
Contributor Author

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.

{title}
{ title }
<a
href={linkUrl}
className={ cx('video-link') }
rel='noopener noreferrer'
target='_blank' >
{ linkUrl }
</a>
</video>
);
}
Expand Down
10 changes: 2 additions & 8 deletions src/components/test/test.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,19 +253,13 @@
}
}

.image-link {
.image-link, .video-link {
display: inline-block;
font-size: 11px;
padding: 0 1em 1em 1em;
}

.image {
display: block;
max-width: 100%;
height: auto;
}

.video {
.image, .video {
display: block;
Copy link
Contributor Author

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.

Copy link
Owner

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.

max-width: 100%;
height: auto;
Expand Down
41 changes: 41 additions & 0 deletions test/spec/components/test/context.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ describe('<TestContext />', () => {
ctxItems: wrapper.find('.test-context-item'),
img: wrapper.find('.test-image'),
imgLink: wrapper.find('.test-image-link'),
video: wrapper.find('.test-video'),
videoLink: wrapper.find('.test-video-link'),
link: wrapper.find('.test-text-link'),
snippet: wrapper.find(CodeSnippet)
};
Expand Down Expand Up @@ -134,6 +136,45 @@ describe('<TestContext />', () => {
expect(snippet).to.have.lengthOf(0);
expect(img).to.have.lengthOf(1);
});

it('renders local video', () => {
const context = '/testvideo.mp4';
const { wrapper, snippet, video } = getInstance({
context: JSON.stringify(context),
className: 'test'
});
expect(wrapper).to.have.className('test');
expect(snippet).to.have.lengthOf(0);
expect(wrapper).to.have.descendants('video');
Copy link
Contributor Author

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)

Copy link
Owner

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');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it

expect(video).to.have.attr('src', '/testvideo.mp4');
});

it('renders video url with protocol', () => {
const context = 'http://test.url.com/testvideo.mp4';
const { wrapper, snippet, videoLink } = getInstance({
context: JSON.stringify(context),
className: 'test'
});
expect(wrapper).to.have.className('test');
expect(snippet).to.have.lengthOf(0);
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 });
Copy link
Owner

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?

Copy link
Contributor Author

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.

Copy link
Owner

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.

});

it('renders image url without protocol', () => {
const context = 'test.url.com/testvideo.mp4';
const { wrapper, snippet, videoLink } = getInstance({
context: JSON.stringify(context),
className: 'test'
});
expect(wrapper).to.have.className('test');
expect(snippet).to.have.lengthOf(0);
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');
});
});

describe('when context is an object', () => {
Expand Down