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.

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

Add Audio Block #2299

Merged
merged 4 commits into from
Aug 11, 2017
Merged

Add Audio Block #2299

merged 4 commits into from
Aug 11, 2017

Conversation

timmyc
Copy link
Contributor

@timmyc timmyc commented Aug 9, 2017

For #804

This branch adds the basics of the audio block. Currently you can select/upload an audio file from the Media Library, or use an external url. As mentioned on the ticket, the audio widget supports additional parameters like adding additional playback types, and looping - which we might consider in future iterations.

Also, perhaps there could potentially be some simple validation on the url field - but taking a page from the video block and trying to simplify things for now.

To Test

  • Create an audio block using an existing mp3 in your media library
  • Create an audio block by uploading a new audio file to your media library
  • Create an audio block using an externally hosted mp3 file.

@BE-Webdesign
Copy link
Contributor

BE-Webdesign commented Aug 9, 2017

Looks good. The extra class works, on the front end but not the back end, don't think that is related to this PR though. One of the tests is not cleaning up correctly and failing. We might need to coordinate with #2309, as the problem lies across both of them. Going to push the fixtures to this branch.

@codecov
Copy link

codecov bot commented Aug 10, 2017

Codecov Report

Merging #2299 into master will increase coverage by 0.02%.
The diff coverage is 36.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2299      +/-   ##
==========================================
+ Coverage   25.82%   25.84%   +0.02%     
==========================================
  Files         156      157       +1     
  Lines        4829     4840      +11     
  Branches      815      817       +2     
==========================================
+ Hits         1247     1251       +4     
- Misses       3024     3029       +5     
- Partials      558      560       +2
Impacted Files Coverage Δ
blocks/library/audio/index.js 36.36% <36.36%> (ø)

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 6d3637f...3ce1560. Read the comment docs.

@mkaz
Copy link
Member

mkaz commented Aug 10, 2017

Tested it out and it worked well for me on inserting from Media Library, but an issue when trying to type in a URL. It would insert immediately on first letter type and fail since h is invalid URL.

Also, you will probably want to add an Upload button next to the Insert, similar to how Image & Gallery are now doing but that probably can come with a version 1.1

@timmyc
Copy link
Contributor Author

timmyc commented Aug 10, 2017

Indeed, good catch @mkaz - thinking we should emulate the embed functionality for verifying urls:

invalid-url

timmyc and others added 4 commits August 11, 2017 07:47
Adding the testing fixtures. To update them, change the core__audo.html
and run `npm run fixtures:regenerate`.
@timmyc timmyc merged commit 2b18b14 into master Aug 11, 2017
@timmyc timmyc deleted the add/audio-block branch August 11, 2017 15:54
@mtias mtias mentioned this pull request Aug 14, 2017
@afercia afercia added the [Feature] Widgets Screen The block-based screen that replaced widgets.php. label Jan 30, 2019
ceyhun pushed a commit that referenced this pull request Jun 17, 2020
…ild_detection

Insure bundle file exists when determining up-to-date status
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Widgets Screen The block-based screen that replaced widgets.php.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants