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

Blocks: Add Upload button to audio and video blocks #5431

Merged
merged 1 commit into from Mar 7, 2018

Conversation

Projects
None yet
4 participants
@gziolo
Copy link
Member

gziolo commented Mar 6, 2018

Description

As suggested by @youknowriad here:

Add an "Upload" button to the audio and video blocks (similar to the image and gallery blocks)

This PR adds the Upload button for the audio and video blocks.

I also added a few refactorings to make code more generic and work properly with the new requirements. mediaUpload util accepts a type of file as a param to work with audio and video formats.

I also fixed a broken behavior of the edit button which did nothing when the placeholder was displayed in the audio or video block. I updated the logic to display the button only when the file is selected.

This should conclude pre-requirements to make it possible to merge #5211.

How Has This Been Tested?

Manually:

  • Make sure there are no regression for the following blocks: image, gallery and cover image.
  • Use new Upload button to add a new audio file in he audio block.
  • Use new Upload button to add a new video file in the video block.
  • Make sure that Edit button is not visible when in the edit mode for audio and video blocks.

Screenshots (jpeg or gifs if applicable):

Please ignore wide and full buttons in the block's toolbar - I used 2 different websites to create screenshots :)

Audio - file selected

screen shot 2018-03-06 at 10 24 47

Audio - editing

Before
screen shot 2018-03-06 at 10 36 35

After
screen shot 2018-03-06 at 10 24 54

Video - file selected

screen shot 2018-03-06 at 10 25 07

Video - editing

Before
screen shot 2018-03-06 at 10 36 55

After
screen shot 2018-03-06 at 10 25 15

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

@gziolo gziolo requested a review from WordPress/gutenberg-core Mar 6, 2018

@gziolo gziolo self-assigned this Mar 6, 2018

@gziolo gziolo referenced this pull request Mar 6, 2018

Merged

Blocks: Try to make MediaUpload component extensible #5211

3 of 3 tasks complete
@@ -25,13 +25,13 @@ import { rawHandler } from '../api';
*/
export default function ImagePlaceholder( { className, icon, label, onSelectImage, multiple = false } ) {

This comment has been minimized.

@youknowriad

youknowriad Mar 6, 2018

Contributor

Do you think, this could be renamed "MediaPlaceholder" to make the experience even more similar? and provide a media type?

This comment has been minimized.

@gziolo

gziolo Mar 6, 2018

Author Member

As a next iteration, we could bring those two closer by doing proper refactoring. Actually, this was the first step I took, but then I backed off, because I didn't want to change too much in one PR.

This comment has been minimized.

@gziolo

gziolo Mar 6, 2018

Author Member

Also see my comment #5431 (comment), you were too fast :)

*/
export function mediaUpload( filesList, onImagesChange ) {
export function mediaUpload( filesList, onImagesChange, allowedType ) {

This comment has been minimized.

@youknowriad

youknowriad Mar 6, 2018

Contributor

Should we rename onImagesChange => onChange or onMediaChange

This comment has been minimized.

@gziolo

gziolo Mar 6, 2018

Author Member

I missed that :(

This comment has been minimized.

@gziolo

gziolo Mar 6, 2018

Author Member

I picked onFileChange to reflect the param's description.

@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Mar 6, 2018

Further improvements to consider

@karmatosed and @jasmussen - are there any plans to iterate further on the media-related blocks? Speaking myself, it seems like this PR bring images and other media formats closer in terms of UX. However, there are a couple of things we could tackle in the follow-up PRs I noticed when testing my changes:

  • Audio & video blocks don't support drag & drop for files. I was able to add that feature with a few lines of code when doing some explorations.
  • The drop area doesn't take into account media type, we would need to add this capability.
  • Should it be possible to specify image using url similar to what we propose in case of audio or video?
  • Should we make Edit button work in a similar fashion for all media types?

@gziolo gziolo requested review from karmatosed and jasmussen Mar 6, 2018

@gziolo gziolo force-pushed the add/upload-button-video-audio branch from 0ee584e to 12b9d5f Mar 6, 2018

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Mar 6, 2018

are there any plans to iterate further on the media-related blocks?

Media is sort of the forgotten space. Yes, there are many plans to improve, including a few tracked in #4108, #2439 and #5256.

It's not for a lack of desire to improve, unify, enhance things around media related blocks, it's purely a lack of resources, and so because they work, they haven't been prioritized.

Your list looks good, though, and I'd thumb up those improvements if they were made.

@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Mar 6, 2018

Your list looks good, though, and I'd thumb up those improvements if they were made.

It's not for this week, but I will keep it on my todo list :)

@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Mar 6, 2018

👍 for this and also agree, this is a small but significant media improvement we can make.

@gziolo gziolo merged commit 0d9ad2c into master Mar 7, 2018

2 checks passed

codecov/project 39.64% (+0.02%) compared to ec8fcba
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo deleted the add/upload-button-video-audio branch Mar 7, 2018

@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Mar 7, 2018

Opened a follow-up issue as discussed above: #5456.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment