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

Rename cover image to cover; Add video in cover block; #10659

Merged
merged 4 commits into from
Oct 18, 2018

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Oct 16, 2018

Description

Implements the video background functionality for cover referred in https://make.wordpress.org/core/2018/09/26/an-update-on-gutenberg-tasks/.
Replaces: #10639

This PR renames cover image block into cover and adds video support o cover.
The approach used to rename the block is the same used for the move of core/text to core/paragraph.
A change in api/parser.js. This approach may be seen as a temporary solution until a final approach to this use case exists, and we feel comfortable in exposing an API for this.

Transformations were updated, and a video transform was added.

How has this been tested?

Verify the cover block still works as expected, containing the same functionality the existed in the cover image.
Verify it is possible to set video as background in the cover block.

Screenshot

oct-18-2018 17-32-42

@jorgefilipecosta jorgefilipecosta added the [Type] Enhancement A suggestion for improvement. label Oct 16, 2018
@jorgefilipecosta jorgefilipecosta added this to the 4.1 - UI freeze milestone Oct 16, 2018
@jorgefilipecosta jorgefilipecosta requested review from a team and mcsf October 16, 2018 20:41
mediaType = VIDEO_BACKGROUND_TYPE;
}
} else { // for media selections originated from existing files in the media library.
mediaType = media.type;
Copy link
Contributor

Choose a reason for hiding this comment

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

What other types are there coming from the library, and why should we trust that media.type is valid for this block? Shouldn't we screen for IMAGE and VIDEO?

Copy link
Member Author

Choose a reason for hiding this comment

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

We set as the allowed media type for the media library video and image so another type being passed would probably be an indicator of a bug in another place. But to be on safe side I added a check that makes sure the type is image or video.

@@ -245,10 +318,10 @@ export const settings = {
className={ className }
labels={ {
title: label,
name: __( 'an image' ),
name: __( 'an image or a video' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should have a translators: note reading something like

translators: Fragment of the sentence: "Drag %s, upload a new one or select a file from your library."

Aside: the way we interpolate these sentence fragments in MediaPlaceholder is pretty bad i18n.

"displayAsDropdown": false,
"showHierarchy": false
"showHierarchy": false,
"showPostCounts": false
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this hunk? :)

if ( 'core/cover-image' === name ) {
name = 'core/cover';
}

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed in DM, we'll need a proper way of dealing with this soon, but I'm not convinced it should become a public interface anytime soon.

@jorgefilipecosta
Copy link
Member Author

Hi @mcsf I think all the feedback provided was applied.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Left some code comments, looks good overall, I'm going to give it a try now.

@@ -58,16 +58,22 @@ const blockAttributes = {
customOverlayColor: {
type: 'string',
},
backgroundType: {
type: 'string',
default: 'image',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use the constant here?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes we should 👍

// Videos contain the media type of 'file' in the object returned from the rest api.
mediaType = VIDEO_BACKGROUND_TYPE;
}
} else { // for media selections originated from existing files in the media library.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's weird that the object shape is different between the MediaLibrary component and the file upload. We should probably normalize one of them. (I think we should use the REST API shape in all cases for now).

This can be done in a separate PR to avoid blocking this though and it has other consequences potentially.

}
mediaType = media.type;
}
if ( mediaType ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really necessary? I feel at this point, we'd always have a mediaType right?

onChange={ toggleParallax }
/>
<PanelBody title={ __( 'Cover Settings' ) }>
{ IMAGE_BACKGROUND_TYPE === backgroundType && (
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great if we could support this for videos at some point.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Feel free to address the minor comments and merge.

Copy link
Member

@karmatosed karmatosed left a comment

Choose a reason for hiding this comment

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

Looks great and I can see people loving this, thanks!

@jorgefilipecosta jorgefilipecosta merged commit c3501c7 into master Oct 18, 2018
@jorgefilipecosta jorgefilipecosta deleted the update/video-in-cover-block branch October 18, 2018 14:07
@paaljoachim
Copy link
Contributor

paaljoachim commented Oct 22, 2018

Is there a way to embed a Youtube video into the Cover block?
Or does it just work for ones own hosted videos for the moment?

From https://make.wordpress.org/test/2018/10/19/call-for-testing-gutenberg-4-1-pre-release
2.Add a video background using the Cover block. Try different browsers. (10659)
Here I expected to find the Cover block in the layout Elements, but noticed it in the Common Blocks (Common blocks is a word that really does not say much) As Layout Elements word would actually be most of the blocks used.

First of all I expected to embed a youtube video here.
I first uploaded one video but it was very slow to upload. It seemed like it did not work I then uploaded another video that used a few seconds before it came into place. There was no spinner telling me that it was working. Right now I am seeing the first video uploaded. There is no sound. It would be good with the option to have audio on or off. Clicking the pencil icon I see the media library but not the videos I uploaded. Opening another tab and refreshing the page I see that the videos are in the media library.

ghost pushed a commit to presscustomizr/hueman that referenced this pull request Oct 25, 2018
…gutenberg#10659, but posts created with the former cover-image block will still use the wp-block-cover-image css class

related to #702
ghost pushed a commit to presscustomizr/customizr that referenced this pull request Oct 25, 2018
… "cover".

See WordPress/gutenberg#10659, but posts created with the former cover-image block will still use the wp-block-cover-image css class.
fixes #1601
antpb pushed a commit to antpb/gutenberg that referenced this pull request Oct 26, 2018
## Description
This commit renames cover image block into cover and adds video support o cover.
The approach used to rename the block is the same used for the move of core/text to core/paragraph.
A change in api/parser.js. This approach may be seen as a temporary solution until a final approach to this use case exists, and we feel comfortable in exposing an API for this.

Transformations were updated, and a video transform was added.

## How has this been tested?
Verify the cover block still works as expected, containing the same functionality the existed in the cover image.
Verify it is possible to set video as background in the cover block.
noErrdev added a commit to noErrdev/Hueman-WordPress-Theme that referenced this pull request Aug 26, 2024
…gutenberg#10659, but posts created with the former cover-image block will still use the wp-block-cover-image css class

related to #702
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants