-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 block examples for all core blocks #17493
Conversation
f8dd569
to
08a039f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some suggestions.
@@ -20,5 +20,10 @@ export const settings = { | |||
align: true, | |||
html: false, | |||
}, | |||
example: { | |||
attributes: { | |||
feedURL: 'https://wordpress.org', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feedURL: 'https://wordpress.org', | |
feedURL: __( 'https://wordpress.org' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change
feedURL: 'https://wordpress.org',
feedURL: __( 'https://wordpress.org' ),
URL's don't need to be translated. 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's usually helpful to translate users to redirect users to the homepage of their locale website like https://hi.wordpress.org/ if they're using WP in that locale.
attributes: { | ||
sizeSlug: 'large', | ||
url: 'https://images.unsplash.com/photo-1549880339-d93e3072aef4', | ||
caption: __( 'Snow covered mountain' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Image(s) from Unsplash probably need a copyright note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should bundle any image we use as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use images hosted on wp.org instead? Bundling could be challenging as the public URL is not easily accessible here.
* | ||
* @return {Object} block. | ||
*/ | ||
export const getBlockFromExample = ( name, example ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be createBlockFromExample
for conistency with the default name and the description?
export const getBlockFromExample = ( name, example ) => { | |
export const createBlockFromExample = ( name, example ) => { |
Co-Authored-By: Sören Wrede <soerenwrede@gmail.com>
This is a great place to add some personality. |
@@ -21,6 +21,12 @@ export const settings = { | |||
description: __( 'Embed a video from your media library or upload a new one.' ), | |||
icon, | |||
keywords: [ __( 'movie' ) ], | |||
example: { | |||
attributes: { | |||
src: 'https://make.wordpress.org/design/files/2019/03/block-manager.mp4', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't play in Safari.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to have a very small auto-playing video instead? It doesn't make sense to load a video that you cannot play.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I found this place for public domain video: https://www.pond5.com/stock-video-footage/1/.html#1/18447/durationlt:10,pricelt:0
This one is cute, albeit short: https://www.pond5.com/stock-footage/44576423/little-smokey-walking-cage.html
But the problem then becomes, can we hotlink it? Where should these things be hosted? Is make.wordpress.org okay?
I think you're right with a dark background. Maybe with a lighter background? Also, I'm not sure why, but I don't get the colored text in my preview. And it's not registering with me visually as a "group". |
I did not see colored text in twenty nineteen. Perhaps something with them registering custom color palettes. |
I'm using the Gutenberg Starter Theme, so not sure if that effects it either. |
I think we should not rely on color palettes for the example and only use custom colors because these work across themes. |
… the copy in the Columns block." This reverts commit 6472d1c.
I reverted the base64 commit because in my testing it increases the bundle size of the block-library script by 1Mb. Let's use URLs for now until we figure out a better approach to bundle these images in Core instead. |
updated the group example to avoid the use of the color palette. It should work well across themes now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this. I think we should scramble to get it in.
On very slow connections it does surface a few improvements we can make to the preview component., but since that's only the first time you hover a block and then never after, I would not call that a blocker. Furthermore, it drastically improves the usability for sighted users of the block library, and it helps encourage 3rd party block developers to include a preview, that it's an important feature to ship.
However I think we should make a quick change. We are hotlinking the images, and we are using Unsplash. The latters license while very liberal, the suggestion to credit photographers makes it perhaps 0.05% less liberal than a strict interpretation of the GPL, which suggests complete and total freedom.
So just in order to be totally kosher, I would recommend replacing all images with certified public domain images from Wikimedia. Using this list https://commons.wikimedia.org/wiki/Featured_Pictures_in_the_Public_Domain I would suggest we could use the following:
- https://commons.wikimedia.org/wiki/File:Windbuchencom.jpg
- https://commons.wikimedia.org/wiki/File:MarsSunset.jpg
- https://commons.wikimedia.org/wiki/File:MarsSunset.jpg
- https://commons.wikimedia.org/wiki/File:M104_ngc4594_sombrero_galaxy_hi-res.jpg
- https://commons.wikimedia.org/wiki/File:Glacial_lakes,_Bhutan.jpg
- https://commons.wikimedia.org/wiki/File:Mount_Hood_reflected_in_Mirror_Lake,_Oregon.jpg
- https://commons.wikimedia.org/wiki/File:Kuznetsk_Alatau_1.jpg
- https://commons.wikimedia.org/wiki/File:Sediment_off_the_Yucatan_Peninsula.jpg
- https://commons.wikimedia.org/wiki/File:Rocks_El_Torcal_de_Antequera_karst_Andalusia_Spain.jpg
- https://commons.wikimedia.org/wiki/File:Icy_shores_of_Lake_Michigan.jpg
- https://commons.wikimedia.org/wiki/File:Drymona_water_fall_and_pool_north_Euboea_Greece.jpg
- https://commons.wikimedia.org/wiki/File:Beech_and_ferns_in_Gullmarsskogen.jpg
An additional benefit is that Wikemedia explicitly permits hotlinking: https://commons.wikimedia.org/wiki/Commons:Reusing_content_outside_Wikimedia/technical
I hope to take a stab at replacing the URLs and returning.
The overall content, other than the images, are great, though, works well:
Okay, pushed good fastloading guaranteed publicdomain images that look good. These five, for reference:
Had to look for Mont Blanc and a specific bird due to the prose around them. A couple more things before we can merge:
I will hopefully be back. |
This video is smaller, waaay faster loading, and it's more generic than showing the Block Manager. Good for the preview, where you can't play back the video anyway.
I just updated the video to be a very short and generic WordPress logo file. This is more appropriate for the preview where you can't even play it back. Plus the preview looks good: With the help of @kjellr I'm investigating why it doesn't play back in Safari, as noted in the reviews. It may be the Make server. |
I have created #17655 to add it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted previously, I really like this. I think we should get it in.
I replaced all the images with Wikimedia images that are public domain. They also allow us to hotlink, so that helps. In addition, the files are small, so they load really fast compared the the previous images, and given they will be shown in scaled down previews the fidelity is fine.
I also removed the video, because the one we had did not play in Safari, and it should be hosted on a CDN. I created #17655 to track this.
What remains here is lovely, and I think it's good to go.
However I also did a bit of work on this, so would appreciate a sanity check, for example from @kjellr.
Just created a Trac ticket to host images on w.org's CDN: https://core.trac.wordpress.org/ticket/48292 |
I know it's a minor issue but, could we address the sizing of Guessing it's due to flex-grow/shrink since the explicit sizing seems to be in place already. |
closes #17488
This PR adds block examples for the core blocks in order to show a preview of the blocks in the inserter help panel. It uses the examples in the #17488 issue.