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

HTML5 Audio elements #508

Merged
merged 12 commits into from Aug 3, 2017

Conversation

Projects
None yet
2 participants
@SteelWagstaff
Contributor

SteelWagstaff commented Jul 25, 2017

Turns links to .mp3, .ogg, and .wav files into HTML5 audio elements

SteelWagstaff added some commits Jul 21, 2017

mp3 -> html5 + H5P embed support
Added support for mp3 -> html5 audio and a primitive h5p embedder.
Adds HTML5 audio embed support for .mp3 links
Code clean up (removed H5P embed test)
Adds support for links -> HTML5 audio elements
Turns links to web-published .mp3, .ogg, and .wav files into HTML5
audio elements
@SteelWagstaff

This comment has been minimized.

Show comment
Hide comment
@SteelWagstaff

SteelWagstaff Jul 25, 2017

Contributor

I'm still a bit new to the pull request process. I think my changes to the media-embedder.js file are all good, but the media-embedder-test.js file still needs work. Sorry about that. Willing to learn more about how to become better at this in future, if someone is able to offer some guidance.

Contributor

SteelWagstaff commented Jul 25, 2017

I'm still a bit new to the pull request process. I think my changes to the media-embedder.js file are all good, but the media-embedder-test.js file still needs work. Sorry about that. Willing to learn more about how to become better at this in future, if someone is able to offer some guidance.

Add media-embedder tests for .mp3, .ogg, .wav files
Added media-embedder tests for .mp3, .ogg, .wav files

@robertknight robertknight self-assigned this Jul 26, 2017

SteelWagstaff added some commits Jul 26, 2017

Cleaned up media-embedder tests
Cleaned up media-embedder tests
More testing cleanup
More cleaning up of the media-embedder tests
@robertknight

This comment has been minimized.

Show comment
Hide comment
@robertknight

robertknight Aug 1, 2017

Contributor

Hi @SteelWagstaff , sorry I haven't gotten to this yet. I'm currently juggling a couple of different projects which have deadlines coming up. I promise I haven't forgotten!

Contributor

robertknight commented Aug 1, 2017

Hi @SteelWagstaff , sorry I haven't gotten to this yet. I'm currently juggling a couple of different projects which have deadlines coming up. I promise I haven't forgotten!

@SteelWagstaff

This comment has been minimized.

Show comment
Hide comment
@SteelWagstaff

SteelWagstaff Aug 1, 2017

Contributor

Sounds good. We were really hoping to begin using both the audio and H5P embeds in mid-August for some projects we hoped would be ready for testing at the start of the upcoming academic semester here (early September). Do you think that would be realistic time-wise, or should we be making contingency plans?

Contributor

SteelWagstaff commented Aug 1, 2017

Sounds good. We were really hoping to begin using both the audio and H5P embeds in mid-August for some projects we hoped would be ready for testing at the start of the upcoming academic semester here (early September). Do you think that would be realistic time-wise, or should we be making contingency plans?

@robertknight

Hi @SteelWagstaff - I've given this a test today and it looks to be working well. The main thing that needs to happen to merge this is to fix the tests. You can run them locally with gulp test-watch.

I made some suggestions on this in the comments. Please let me know if you need a hand.

Fixed tests
Fixed tests for audio elements.
@robertknight

This comment has been minimized.

Show comment
Hide comment
@robertknight

robertknight Aug 2, 2017

Contributor

Do you think that would be realistic time-wise, or should we be making contingency plans?

I think we should be able to finish up the audio support pretty quickly. H5P I think is possible but needs more care/thought in order not to create a gaping security hole in the client. I'll need a little bit more time to have a think about this and do some tests.

Contributor

robertknight commented Aug 2, 2017

Do you think that would be realistic time-wise, or should we be making contingency plans?

I think we should be able to finish up the audio support pretty quickly. H5P I think is possible but needs more care/thought in order not to create a gaping security hole in the client. I'll need a little bit more time to have a think about this and do some tests.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 2, 2017

Codecov Report

Merging #508 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #508      +/-   ##
==========================================
- Coverage   90.82%   90.81%   -0.01%     
==========================================
  Files         134      134              
  Lines        5371     5380       +9     
  Branches      936      937       +1     
==========================================
+ Hits         4878     4886       +8     
- Misses        493      494       +1
Impacted Files Coverage Δ
src/sidebar/media-embedder.js 94.44% <100%> (+0.79%) ⬆️
src/sidebar/components/markdown.js 91.78% <0%> (-1.37%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8e523f...ce7e2d6. Read the comment docs.

codecov bot commented Aug 2, 2017

Codecov Report

Merging #508 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #508      +/-   ##
==========================================
- Coverage   90.82%   90.81%   -0.01%     
==========================================
  Files         134      134              
  Lines        5371     5380       +9     
  Branches      936      937       +1     
==========================================
+ Hits         4878     4886       +8     
- Misses        493      494       +1
Impacted Files Coverage Δ
src/sidebar/media-embedder.js 94.44% <100%> (+0.79%) ⬆️
src/sidebar/components/markdown.js 91.78% <0%> (-1.37%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8e523f...ce7e2d6. Read the comment docs.

@SteelWagstaff

This comment has been minimized.

Show comment
Hide comment
@SteelWagstaff

SteelWagstaff Aug 2, 2017

Contributor

Yes--I agree re: H5P and iFrame support. Let me know if it'd be helpful to talk through any of this as you think on it. We're obviously keen on using it, but recognize the need for caution and security-mindedness here.

Contributor

SteelWagstaff commented Aug 2, 2017

Yes--I agree re: H5P and iFrame support. Let me know if it'd be helpful to talk through any of this as you think on it. We're obviously keen on using it, but recognize the need for caution and security-mindedness here.

Test fix
Tiny fix to the test to account for URLs that may have had uppercase
letters (the audio embed converts to lowercase to account for links to
.MP3 files rather than .mp3)
Fixes for Pull Request
Made changes suggested in code review:
#508 (review)
.
assert.equal(element.childElementCount, 1);
assert.equal(element.children[0].tagName, 'AUDIO');
assert.equal(element.children[0].src, url.toLowerCase());

This comment has been minimized.

@robertknight

robertknight Aug 3, 2017

Contributor

Re. the toLowerCase here, some parts of URLs are case sensitive and others are not. Strictly speaking, the way to compare URLs ignoring case in only the right places would be to parse the URL and reformat it. However, I think this is fine for the purposes of this test.

new URL('HTTPS://WWW.GOOGLE.COM/FOO/BAR?BAZ#WIBBLE').href
// "https://www.google.com/FOO/BAR?BAZ#WIBBLE"
@robertknight

robertknight Aug 3, 2017

Contributor

Re. the toLowerCase here, some parts of URLs are case sensitive and others are not. Strictly speaking, the way to compare URLs ignoring case in only the right places would be to parse the URL and reformat it. However, I think this is fine for the purposes of this test.

new URL('HTTPS://WWW.GOOGLE.COM/FOO/BAR?BAZ#WIBBLE').href
// "https://www.google.com/FOO/BAR?BAZ#WIBBLE"
@robertknight

This comment has been minimized.

Show comment
Hide comment
@robertknight

robertknight Aug 3, 2017

Contributor

Looks good to me 👍

Contributor

robertknight commented Aug 3, 2017

Looks good to me 👍

@robertknight robertknight merged commit f43fa51 into hypothesis:master Aug 3, 2017

3 checks passed

codecov/patch 100% of diff hit (target 90.82%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +9.17% compared to c8e523f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment