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

Pinterest block #13905

Merged
merged 16 commits into from
Nov 7, 2019
Merged

Pinterest block #13905

merged 16 commits into from
Nov 7, 2019

Conversation

pento
Copy link
Contributor

@pento pento commented Oct 31, 2019

This PR adds a Pinterest block, providing block-editor native equivalent functionality of the existing Pinterest embed.

It includes the ability to automatically embed Pinterest URLs that are pasted into the editor, and it can convert Pinterest embeds in the Classic block to Pinterest blocks.

Screen recording of the Pinterest block

Changes proposed in this Pull Request:

  • This PR adds a new block, jetpack/pinterest.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

This is an existing feature being ported to the block editor.

Internal reference: pb5gDS-af-p2

Testing instructions:

This is currently marked as a beta block, you will need to add define( 'JETPACK_BETA_BLOCKS', true ); to your wp-config.php file.

  • Open the Block Editor.
  • Try pasting various Pinterest URLs.
  • Try editing the embeds.
  • Try converting classic posts with Pinterest embeds to block posts.

Proposed changelog entry for your changes:

  • Block Editor: Added a Pinterest block.

@pento pento added [Feature] Shortcodes / Embeds [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 31, 2019
@pento pento added this to the 8.0 milestone Oct 31, 2019
@pento pento requested a review from a team October 31, 2019 05:56
@pento pento self-assigned this Oct 31, 2019
@jetpackbot
Copy link

jetpackbot commented Oct 31, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: December 3, 2019.
Scheduled code freeze: November 26, 2019

Generated by 🚫 dangerJS against 794137c

@scruffian
Copy link
Member

Tested with single pins, boards and users ✅
Also tested transforming from a classic block ✅

Code looks good, just a few minor comments. Might be interesting to see if we can generalise some of this in the future if we end up with a bunch of embed blocks....

extensions/blocks/pinterest/edit.js Outdated Show resolved Hide resolved
extensions/blocks/pinterest/edit.js Outdated Show resolved Hide resolved
extensions/blocks/pinterest/edit.js Show resolved Hide resolved
extensions/blocks/pinterest/index.js Outdated Show resolved Hide resolved
extensions/blocks/pinterest/pinterest.php Outdated Show resolved Hide resolved
extensions/blocks/pinterest/edit.js Show resolved Hide resolved
@jeherve jeherve added [Block] Pinterest [Type] Feature Request [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Feature] Shortcodes / Embeds [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 31, 2019
@MichaelArestad
Copy link
Contributor

Works pretty well. The only issue I had was when I tried to use a link generated from a Pin's Share menu. Here's an example: https://pin.it/h5ihqcvazkeanu

It fails gracefully, but doesn't tell me what I need to do to share the pin.

@MichaelArestad
Copy link
Contributor

MichaelArestad commented Oct 31, 2019

Double checked. This needs a preview. You can see an example of how to add a preview here:

https://github.com/Automattic/jetpack/blob/master/extensions/blocks/gif/index.js#L61

The preview will show up when hovering over the block using the Block Inserter in the Top Bar.

image

@simison
Copy link
Member

simison commented Oct 31, 2019

I think there's a [pinterest] shortcode in Jetpack (https://jetpack.com/support/shortcode-embeds/) so you could add a shortcode transform. Not a blocker for merging. :-)

@pento
Copy link
Contributor Author

pento commented Oct 31, 2019

Thanks for the reviews, everyone! I'll work through the feedback today. 🙂

Might be interesting to see if we can generalise some of this in the future if we end up with a bunch of embed blocks

We can do one better, and handle it in Core (WordPress/gutenberg#13490). I can definitely see it being a helpful equivalent of Core's older PHP-based embed registering APIs.

@pento
Copy link
Contributor Author

pento commented Oct 31, 2019

I think there's a [pinterest] shortcode in Jetpack (jetpack.com/support/shortcode-embeds) so you could add a shortcode transform. Not a blocker for merging. :-)

Is there some magic in Jetpack that automatically registers shortcodes? It isn't registered in /modules/shortcodes/pinterest.php, where other files in that directory seem to explicitly call add_shortcode().

@pento
Copy link
Contributor Author

pento commented Oct 31, 2019

Added a block preview in 5fffbb876.

Screenshot of the Pinterest block preview

Note that the vertical alignment is weird due to WordPress/gutenberg#18134.

@pento
Copy link
Contributor Author

pento commented Oct 31, 2019

The only issue I had was when I tried to use a link generated from a Pin's Share menu. Here's an example: https://pin.it/h5ihqcvazkeanu

Good catch, thanks @MichaelArestad! I'm going to have to think about this a bit more: it currently won't work, as we need the full URL to determine what kind of embed it is. But, we can't get the full URL without a call to the server to resolve it (which is possible, but sub-optimal, since it would stop us from adding this block to the block directory).

@MichaelArestad
Copy link
Contributor

@pento NICE PREVIEW. The alignment thing is a bummer, but I suspect that will get fixed up as it likely will be a common problem.

@pento
Copy link
Contributor Author

pento commented Nov 4, 2019

d702bf37f is a fairly wild solution for supporting pin.it URLs: it adds a new /wpcom/v2/resolve-redirect/<url> REST API endpoint, which given a url, will fetch the URL, follow any redirects, and return the final destination.

The Pinterest block is able to wait for the response, then replace the pin.it URL with the resolved pinterest.com URL.

I'd appreciate a sanity check on whether this is overkill or not. It's worth noting that, due to CORS rules, it's not possible to find the redirect destination from the browser, it has to be done from the server.

@tellyworth: Could I get your thoughts on whether the PHP used in this block is appropriate for the block directory? The two uses are the new REST API endpoint I just added, and jetpack_pinterest_block_load_assets(), which runs as the block's render_callback. This ensures that the Pinterest embed script is only loaded once per page.

@jeherve
Copy link
Member

jeherve commented Nov 5, 2019

It isn't registered in /modules/shortcodes/pinterest.php, where other files in that directory seem to explicitly call add_shortcode().

Although it's in the modules/shortcodes directory, that one is only an embed, i.e. changing URLs into embeds.

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello pento! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D35021-code before merging this PR. Thank you!

@jeherve
Copy link
Member

jeherve commented Nov 5, 2019

It seems to be failing on the extensions/index.json change, but I have no idea why.

D35021-code should get us started. You'll need to add the new files to the diff manually though, as Fusion does not yet support auto-adding new files.

jeherve
jeherve previously approved these changes Nov 6, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This looks good to me. I only have 4 questions / remarks below. I think those could be addressed in a follow-up PR.


For some reason in some scenarios the API call was using an undefined value instead of the URL, and I received no useful error message in the interface to let me know that something had failed. Trying again immediately worked:
https://videopress.com/v/HE2udtWu


Maybe as a V2 we could add a size option to the block sidebar, since Pinterest apparently allows you to set a data-pin-width attribute to control the size of the embed.
I could also imagine some folks wanting to show more of their board instead of the first 700px of it or so, but I am not sure that's an option offered by Pinterest.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 6, 2019
@pento
Copy link
Contributor Author

pento commented Nov 7, 2019

For some reason in some scenarios the API call was using an undefined value instead of the URL, and I received no useful error message in the interface to let me know that something had failed. Trying again immediately worked:
videopress.com/v/HE2udtWu

Well, that was an interesting reminder of how React batches state updates together. 🙂 Fixed in 794137c.

Maybe as a V2 we could add a size option to the block sidebar, since Pinterest apparently allows you to set a data-pin-width attribute to control the size of the embed.

Yah, I agree that additional options probably fall under a V2.

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This looks good now. 👍

@jeherve jeherve merged commit 63716c4 into master Nov 7, 2019
@jeherve jeherve deleted the feature/pinterest-block branch November 7, 2019 16:37
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 7, 2019
@jeherve jeherve added the [Status] Needs Testing We need to add this change to the testing call for this month's release label Nov 7, 2019
@jeherve
Copy link
Member

jeherve commented Nov 7, 2019

@pento If you'd like to see wider testing of the new block to get more feedback, you can follow the instructions and template from PCYsg-iiu-p2 (See testing section) to post a call for testing.

@pento
Copy link
Contributor Author

pento commented Nov 7, 2019

Thanks, @jeherve!

I'm going to leave the call for testing for a little bit, since I'm also discussing with the Pinterest team the possibility of them adding an oEmbed endpoint: if they're able to do that before the Jetpack 8.0 code freeze, I'll re-work this block a bit to use that method for embedding, instead. Since that would require a whole new round of testing, I'd prefer to not put out two calls for testing on the same thing. 🙂

@jeherve jeherve added [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 Nov 8, 2019
jeherve added a commit that referenced this pull request Nov 8, 2019
@huguesvincent
Copy link

you can follow the instructions and template from PCYsg-iiu-p2 (See testing section) to post a call for testing.
Could you point me to this @jeherve. Not sure where to find this template Jeremy... Thanks for your guidance.

As a side note, I sent a follow up to Pinterest so that we can get a definitive direction on this shortly.

jeherve added a commit that referenced this pull request Nov 15, 2019
jeherve added a commit that referenced this pull request Nov 25, 2019
* 8.0 Release: running changelog

* Changelog: add #13921

* Changelog: add #13980

* Changelog: add #13905

* Changelog: add #13971

* Changelog: add #13984

* Changelog: add #14009

* Changelog: add #13620

* Remove things that will ship in 7.9.1

* Changelog: add 7.9.1 release (#14044)

* Changelog: add base for 7.9.1 release

* Update release date and post link

* Changelog: add #14066

* Update changelog for 7.9.1

* Changelog: add #13405

* Changelog: add #13841

* Changelog: add #13924

* Changelog: add #13986

* Changelog: add #14010, #14028, #14053, #14055.

* Changelog: add #14054

* Changelog: add #14031

* Changelog: add #14039

* Changelog: add #14050

* Changelog: add #14070

* Changelog: add #14082

* Changelog: add #14084

* Changelog: add #14111

* Changelog: add #13961

* Changelog: add #14047

* Changelog: add #14091

* Changelog: add #14108

* Changelog: add #14121
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Pinterest [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files [Type] Feature Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants