Giphy Block Registration #10989

jeffersonrabb · 2018-12-17T13:52:12Z

Registers the Giphy block and handles Server-Side Rendering. SSR approach is being used because IFRAME element will be stripped out of post content on WP.com. Discussion of this here: p5TWut-gm-p2.

Companion wp-calypso PR: Automattic/wp-calypso#29638

Changes proposed in this Pull Request:

Registers the Giphy block
Callback to render the Giphy block.

Testing instructions:

Connect Jetpack
Navigate to Settings->Jetpack Constants, check JETPACK_BETA_BLOCKS, and Save.

Add the block
Go to Giphy, find any Gif you like, copy the URL and paste into the block. The gif should show up.
Paste in a Giphy URL following this syntax: http://i.giphy.com/4ZFekt94LMhNK.gif. Past the URL into the block. The gif should show up.
Type any text in to the search field. DIfferent Gifs should show up for anything you search for.
Verify that the text field appears when you click on the block.
Verify that the text field disappears after 3.5 seconds of inactivity, unless no search text has been provided in which case it remains visible.
Verify caption behaves much like the core Image block.
Verify block alignment options all work as expected.

Complete testing instructions found here: Automattic/wp-calypso#29638

Changelog

Block editor: addition of a new Giphy block.

matticbot · 2018-12-17T13:52:18Z

D22406-code. (newly created revision)

jetpackbot · 2018-12-17T13:55:21Z

	Warnings
⚠️	"Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is automated check which relies on PULL_REQUEST_TEMPLATE.We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against d0eb1b5

…tripped out.

…, inclusion of '%' in the saved attribute.

modules/blocks.php

- Remove parens around echo - Add missing style quotes - Use double quotes consistently

sirreal · 2019-01-23T12:15:07Z

I pushed a commit to clean up the HTML a bit. There were some missing as well as mixed single and double quotes.

I removed the parens around the echo arguments, they're not necessary and they seem to be mostly omitted in this code base.

modules/blocks.php

sirreal · 2019-01-23T17:18:06Z

modules/blocks.php

+			<iframe src="<?php echo esc_attr( $giphy_url ); ?>" title="<?php echo esc_attr( $search_text ); ?>"></iframe>
+		</figure>
+		<?php if ( $caption ) : ?>
+			<p class="wp-block-jetpack-giphy-caption"><?php echo wp_kses_post( $caption ); ?></p>


Highlighting for reviewers the use of wp_kses_post here.

sirreal

This looks good and tests well. I have a lingering doubt but it can be addressed in a follow-up PR.

modules/blocks.php

jeherve

I only have minor remarks about this PR (see below), but I do have some feedback about the block itself.

Should Giphy be capitalized in the block description?

For some reason the additional classes don't get added to the wrapper:

I was a bit surprised by the block icon; I would have expected it to be an image or a Giphy logo instead of this:

modules/blocks.php

jeffersonrabb · 2019-01-24T13:08:51Z

I was a bit surprised by the block icon; I would have expected it to be an image or a Giphy logo instead of this:

cc: @thomasguillot

jeherve · 2019-01-24T13:27:33Z

since these all relate to wp-calypso would you mind filing as issues and I'll address them immediately?

Sure thing.

Automattic/wp-calypso#30383
Automattic/wp-calypso#30385

…lypso#30385

… default Giphy URL.

jeffersonrabb · 2019-01-24T20:41:09Z

I have a lingering doubt but it can be addressed in a follow-up PR.

This "lingering doubt" is the placeholder state, correct? If so, it's addressed in: Automattic/wp-calypso#30402 and bd0a281

sirreal

I left one suggestion.

modules/blocks.php

This reverts commit 94392ce.

Addressed.

sirreal

I think this is in a good place and the serialized block structure has likely reached some stability. I think this is ready to land any time.

jeherve

I like it. Let's ship it!

https://giphy.com/gifs/ship-wood-launch-ACqTaHVTEvEZO

spacedmonkey · 2019-02-11T14:19:24Z

For anyone interested, I have open a ticket on core and on the gutenberg repo.

WordPress/gutenberg#13426
https://core.trac.wordpress.org/ticket/46067

jeffersonrabb added the [Status] In Progress label Dec 17, 2018

jeffersonrabb requested a review from a team December 17, 2018 13:52

matticbot added the Touches WP.com Files label Dec 17, 2018

jeherve added the [Block] GIF label Dec 17, 2018

jeherve added the [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack label Jan 9, 2019

jeffersonrabb force-pushed the try/giphy branch from 9179d10 to d299d1d Compare January 16, 2019 22:34

jeffersonrabb added 2 commits January 17, 2019 09:30

Register Giphy block.

13cb590

Giphy block, server-side rendering to protect the iFrame from being s…

e909277

…tripped out.

jeffersonrabb force-pushed the try/giphy branch from e592f5e to e909277 Compare January 17, 2019 15:25

jeffersonrabb requested a review from sirreal January 21, 2019 14:50

jeffersonrabb added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Jan 21, 2019

Change to top padding handling. Attribute name change to 'paddingTop'…

181de07

…, inclusion of '%' in the saved attribute.

sirreal reviewed Jan 23, 2019

View reviewed changes

modules/blocks.php Outdated Show resolved Hide resolved

Clean up quotes and parens

3bc115e

- Remove parens around echo - Add missing style quotes - Use double quotes consistently

This was referenced Jan 23, 2019

Giphy block: Include editable caption? Automattic/wp-calypso#30345

Closed

Giphy block: Use valid figure and figcaption Automattic/wp-calypso#30348

Closed

jeffersonrabb and others added 2 commits January 23, 2019 10:45

Change caption to P from FIGCAPTION, add new class.

5a9fc9e

Use wp_kses_post to escape caption HTML

c412741

sirreal reviewed Jan 23, 2019

View reviewed changes

modules/blocks.php Outdated Show resolved Hide resolved

sirreal reviewed Jan 23, 2019

View reviewed changes

sirreal previously approved these changes Jan 23, 2019

View reviewed changes

sirreal reviewed Jan 24, 2019

View reviewed changes

modules/blocks.php Outdated Show resolved Hide resolved

modules/blocks.php Outdated Show resolved Hide resolved

jeherve previously requested changes Jan 24, 2019

View reviewed changes

modules/blocks.php Outdated Show resolved Hide resolved

modules/blocks.php Outdated Show resolved Hide resolved

jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 24, 2019

Rename Giphy block to Gif.

5cb1cae

jeffersonrabb dismissed sirreal’s stale review via 5cb1cae January 24, 2019 18:01

jeffersonrabb added 2 commits January 24, 2019 13:22

Support for Additional CSS Class in Gif block. Fixes Automattic/wp-ca…

ef7603b

…lypso#30385

Do not render block if Giphy URL is unset. This approach replaces the…

bd0a281

… default Giphy URL.

jeffersonrabb added 3 commits January 24, 2019 15:42

Use esc_url() to sanitize Giphy URL.

71d7d45

Check for existence of top padding attribute before use.

27491b4

Renamed function to better describe use.

091c709

sirreal reviewed Jan 25, 2019

View reviewed changes

modules/blocks.php Show resolved Hide resolved

modules/blocks.php Show resolved Hide resolved

sirreal added 2 commits January 25, 2019 09:57

Abort if paddingTop is missing

94392ce

Revert "Abort if paddingTop is missing"

d0eb1b5

This reverts commit 94392ce.

sirreal approved these changes Jan 25, 2019

View reviewed changes

jeherve added this to the 7.0 milestone Jan 25, 2019

jeherve approved these changes Jan 25, 2019

View reviewed changes

jeherve merged commit a97470e into master Jan 25, 2019

matticbot added the [Status] Needs Changelog label Jan 25, 2019

jeherve deleted the try/giphy branch January 25, 2019 10:08

jeherve added [Status] Needs Testing We need to add this change to the testing call for this month's release [Status] Has Changelog and removed [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Jan 25, 2019

jeherve added a commit that referenced this pull request Jan 25, 2019

Changelog: add #10989

71694fa

spacedmonkey mentioned this pull request Feb 11, 2019

Add a giphy gutenberg block. WordPress/gutenberg#13426

Open

bluefuton mentioned this pull request Mar 21, 2023

Giphy block: unable to select from 'Related GIFs' Automattic/wp-calypso#74546

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Giphy Block Registration #10989

Giphy Block Registration #10989

jeffersonrabb commented Dec 17, 2018 •

edited by jeherve

Loading

matticbot commented Dec 17, 2018

jetpackbot commented Dec 17, 2018 •

edited

Loading

sirreal commented Jan 23, 2019

sirreal Jan 23, 2019

sirreal left a comment

jeherve left a comment

jeffersonrabb commented Jan 24, 2019

jeherve commented Jan 24, 2019

jeffersonrabb commented Jan 24, 2019

sirreal left a comment

sirreal left a comment

jeherve left a comment

spacedmonkey commented Feb 11, 2019

Giphy Block Registration #10989

Giphy Block Registration #10989

Conversation

jeffersonrabb commented Dec 17, 2018 • edited by jeherve Loading

Changes proposed in this Pull Request:

Testing instructions:

Changelog

matticbot commented Dec 17, 2018

jetpackbot commented Dec 17, 2018 • edited Loading

sirreal commented Jan 23, 2019

sirreal Jan 23, 2019

Choose a reason for hiding this comment

sirreal left a comment

Choose a reason for hiding this comment

jeherve left a comment

Choose a reason for hiding this comment

jeffersonrabb commented Jan 24, 2019

jeherve commented Jan 24, 2019

jeffersonrabb commented Jan 24, 2019

sirreal left a comment

Choose a reason for hiding this comment

sirreal left a comment

Choose a reason for hiding this comment

jeherve left a comment

Choose a reason for hiding this comment

spacedmonkey commented Feb 11, 2019

jeffersonrabb commented Dec 17, 2018 •

edited by jeherve

Loading

jetpackbot commented Dec 17, 2018 •

edited

Loading