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

Eventbrite block #14075

Merged
merged 63 commits into from
Jan 24, 2020
Merged

Eventbrite block #14075

merged 63 commits into from
Jan 24, 2020

Conversation

creativecoder
Copy link
Contributor

@creativecoder creativecoder commented Nov 19, 2019

Changes proposed in this Pull Request:

  • Adds jetpack/eventbrite block

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

  • New feature, see pb5gDS-aZ-p2

The goal is to release it as a beta block first, in Jetpack 8.2.

Testing instructions:

Inserting the block

  1. Make sure beta blocks are enabled define( 'JETPACK_BETA_BLOCKS', true );
  2. Insert the Eventbrite block in a page or post and paste an Eventbrite checkout embed code
  3. Save the page/post
  4. Eventbrite event embed should display in the page/post content
  5. Custom eventbrite urls should also work.
  6. Test In-page and Modal embed types.

Transforms

  1. Paste an Eventbrite event url into the block inserter (regular or custom)
  2. The block should transform into an Eventbrite block

Invalid URLs

  1. Insert an Eventbrite block
  2. Paste a URL that isn't an Eventbrite event, or an event that doesn't exist
  3. You should see an error and an option to convert the block into a link

Proposed changelog entry for your changes:

  • Block editor: added an Eventbrite block to embed event checkout

@matticbot
Copy link
Contributor

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

@jetpackbot
Copy link

jetpackbot commented Nov 19, 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: February 11, 2020.
Scheduled code freeze: February 4, 2020

Generated by 🚫 dangerJS against 338da38

@jeherve jeherve added the [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack label Nov 19, 2019
@matticbot
Copy link
Contributor

creativecoder, Your synced wpcom patch D35679-code has been updated.

2 similar comments
@matticbot
Copy link
Contributor

creativecoder, Your synced wpcom patch D35679-code has been updated.

@matticbot
Copy link
Contributor

creativecoder, Your synced wpcom patch D35679-code has been updated.

@creativecoder
Copy link
Contributor Author

@pento I'm haven't yet successfully generated the embed output from the save function in index.js.

  • I originally tried to use <SandBox>, though the iframe content always ends up empty, I think because of the way the content is inserted with document.write, but I haven't verified that.
  • Current approach attempts using dangerouslySetInnerHTML is not working

(I suppose we could always fall back to a PHP rendered, if necessary)

@matticbot
Copy link
Contributor

creativecoder, Your synced wpcom patch D35679-code has been updated.

@pento
Copy link
Contributor

pento commented Nov 20, 2019

We need to render this block in PHP, as the <script> tag can't be saved to the post content.

Using a combination of wp_enqueue_script() and wp_add_inline_script() ensures that eb_widgets.js is only loaded once, and that the widgets are initialised after that script is loaded.

@creativecoder
Copy link
Contributor Author

We need to render this block in PHP, as the <script> tag can't be saved to the post content.

Of course 🤦‍♂ !

@matticbot
Copy link
Contributor

creativecoder, Your synced wpcom patch D35679-code has been updated.

4 similar comments
@matticbot
Copy link
Contributor

creativecoder, Your synced wpcom patch D35679-code has been updated.

@matticbot
Copy link
Contributor

creativecoder, Your synced wpcom patch D35679-code has been updated.

@matticbot
Copy link
Contributor

creativecoder, Your synced wpcom patch D35679-code has been updated.

@matticbot
Copy link
Contributor

creativecoder, Your synced wpcom patch D35679-code has been updated.

@pento
Copy link
Contributor

pento commented Dec 13, 2019

I've been playing around with WordPress/gutenberg#19113 today, which would expose Gutenberg's embed block scaffolding, which we could then make use of here.

Of course, Jetpack has back compat requirements, so we can't just start using it, but as this is a new package, I was thinking it would be worth investigating whether we could ship it with Jetpack, and load it on older WordPress versions, where @wordpress/embed-block isn't available.

@creativecoder
Copy link
Contributor Author

I've been playing around with WordPress/gutenberg#19113 today, which would expose Gutenberg's embed block scaffolding, which we could then make use of here.

@pento Given the number of embeds we're working on, that seems like a great idea.

I tried all manner of npm link, yarn link and file:../ imports to try and use @wordpress/embed-block package from the gutenberg repo in jetpack, but couldn't get any of them to work.

I ended up symlinking extensions/blocks/embed-block in jetpack to the packages/embed-block/src/ folder in the gutenberg repo to get things working locally.

I started with importing the EmbedPreview block, but it's not working at the moment (haven't had much time to look into it yet).

@pento
Copy link
Contributor

pento commented Dec 16, 2019

I've updated the PR to use @wordpress/embed-block.

Note that, at the moment, this PR requires the Gutenberg plugin to be installed and activated: loading @wordpress/embed-block against the block editor in Core causes all sorts of weirdness. 🙂

For running a Docker environment with Jetpack and Gutenberg, I use the Jetpack environment, with a custom docker-compose.jetpack.yml file:

version: '3.3'
services:
  wordpress:
    volumes:
      - /Users/pento/Projects/gutenberg:/var/www/html/wp-content/plugins/gutenberg

I start the environment with this command in the Jetpack directory:

yarn docker:compose -f ../docker-compose.jetpack.yml up -d

* @param WP_REST_Request $request Request used to generate the response.
* @return WP_HTTP_Response|object|WP_Error The REST Request response.
*/
function jetpack_filter_oembed_result( $response, $handler, $request ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is copied from the Gutenberg plugin, because embeds fail when the plugin isn't enabled, as Core currently only allows proper oEmbeds to use this REST API endpoint.

I've left it in for now, (even though the PR doesn't run without the plugin), so I don't forget and wonder why everything doesn't work sometime later. 🙃

@creativecoder
Copy link
Contributor Author

For running a Docker environment with Jetpack and Gutenberg, I use the Jetpack environment, with a custom docker-compose.jetpack.yml file:

@pento Thanks for the instructions--this worked for me without issue. (I wasn't thinking about the @wordpress/embed-block to wp.embedBlock conversion during build--I think that's why I was having so much trouble.)

Here's a quick screen capture of how this works, for posterity.

Eventbrite Block with embed-block

@creativecoder
Copy link
Contributor Author

@davemart-in See above. Moving to the embed block style means we're using the event URL rather than an embed code snippet--my hunch is that this will be easier for users, but wanted to flag for you since it's different than the original design mockup. Options, like to use the checkout embed as a modal or inline, will be in the sidebar.

@creativecoder
Copy link
Contributor Author

creativecoder commented Dec 16, 2019

@pento I've updated with a Pinterest copy/paste 😄

Still to do

  • EVENTBRITE_URL_REGEX and URL_REGEX are not tested
  • Extract event id from url
  • We need an Eventbrite icon
  • The embed preview in the block
  • Figure out modal button options

@pento
Copy link
Contributor

pento commented Dec 17, 2019

Thanks for the start, @creativecoder! Here's what I got done today:

  • Added an icon.
  • As you suggested, renamed the block to "Eventbrite Tickets". Perhaps "Eventbrite Checkout" would be better, since tickets is kind of ambiguous for a service that sells tickets?
  • Previews in the editor now work.
  • The widget now displays on the published page.

Some issues I didn't get to address during my day:

  • Switching the widget type between Modal and Inline doesn't resize the block correctly.
  • Something is causing a scrollbar in the Inline preview.
  • The Modal preview has no styling.
  • cannotEmbed should be set by testing against the URL regex.

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

Copons commented Jan 23, 2020

I think I've addressed all the things! ✨

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.

I think 25e0fb9 may have broken things a bit. I can't seem to get a matching event ID from links like:

https://www.eventbrite.com/e/dex-songwriting-expo-tickets-71249967571?aff=ebdshpfprimarybucket

Does it work for you?

The redirect returns the expected response:

{"url":"https:\/\/www.eventbrite.com\/e\/dex-songwriting-expo-tickets-71249967571","status":200}

But no event ID gets set as a block attribute after that.

@jeherve 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, 2020
@Copons
Copy link
Contributor

Copons commented Jan 24, 2020

Oooh awesome catch @jeherve!
It was caused by the fact that the ID regex /(\d+)\/?\s*$/ matches groups of numbers at the end of the string, and of course fails when the URL has any other chars after the ID, such as query params.

The resolveUrl method takes care of stripping the query off the URL.
Before, it wasn't a big deal, because the regex was only used at the end of the whole process, on the final URL.

My update instead used the regex as soon as the URL was inserted in the input field, which is before resolveUrl, and I totally missed that the URL was set again inside resolveUrl as well.

In 338da38 I've added eventId to the setAttributes inside resolveUrl, and now it works with Eventbrite URLs with query params as well.

@Copons Copons added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 24, 2020
@Copons Copons self-assigned this Jan 24, 2020
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.

I have a few last comments, but I don't think any of those are blockers for a first release. Let's try to address those in follow-up PRs?

import './editor.scss';

const MODAL_BUTTON_STYLES = [
{ name: 'fill', label: __( 'Fill' ), isDefault: true },
Copy link
Member

Choose a reason for hiding this comment

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

You'll probably want to make those strings translatable. That's not a blocker though, can be done later.

Copy link
Contributor

@Copons Copons Jan 24, 2020

Choose a reason for hiding this comment

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

Tracked by #14458

extensions/blocks/eventbrite/edit.js Show resolved Hide resolved
</form>

<div className="components-placeholder__learn-more">
<ExternalLink href="http://en.support.wordpress.com/wordpress-editor/blocks/eventbrite-block/">
Copy link
Member

Choose a reason for hiding this comment

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

We'll need a conditional here so we can have 2 support links, e.g.

const supportLink =

Copy link
Contributor

Choose a reason for hiding this comment

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

Tracked by #14459

},
edit,
save,
transforms: {
Copy link
Member

Choose a reason for hiding this comment

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

This is not working for me for some reason:

https://videopress.com/v/zm4Bbdrg

Copy link
Contributor

Choose a reason for hiding this comment

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

Tracked by #14460

);
}

export default function save( { attributes } ) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be nice to return a link to the event inside the div / button. Right now, the empty container won't be of much help in subscription emails or if Jetpack gets deactivated.

<div class="wp-block-jetpack-eventbrite"><button id="eventbrite-widget-88665223069" class="wp-block-button__link" type="button">Test one two</button></div>
<div id="eventbrite-widget-88665223069" class="wp-block-jetpack-eventbrite"></div>

Copy link
Contributor

Choose a reason for hiding this comment

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

Tracked by #14462

@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 Jan 24, 2020
@jeherve jeherve merged commit 5fcf116 into master Jan 24, 2020
@jeherve jeherve deleted the add/eventbrite-block branch January 24, 2020 12:38
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jan 24, 2020
@jeherve jeherve added the [Status] Needs Testing We need to add this change to the testing call for this month's release label Jan 24, 2020
@jeherve
Copy link
Member

jeherve commented Jan 24, 2020

Ran into another issue when testing:

#14461

@jeherve
Copy link
Member

jeherve commented Jan 24, 2020

r202093-wpcom

@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 Jan 27, 2020
jeherve added a commit that referenced this pull request Jan 27, 2020
jeherve added a commit that referenced this pull request Jan 28, 2020
* [not verified] Remove empty readme section

* Initial changelog for 8.2

* Changelog: add #14220

* Changelog: add #14252

* Changelog: add #14291

* Changelog: add #14309

* Changelog: add #14304

* Changelog: add general connection log.

* Changelog: add #14275

* Changelog: add #14313

* Changelog: add #14213

* Changelog: add #14357

* Add sync testing instructions

* Add 8.1.1 changelog back

See eeaafab and 61757eb

* Changelog: add #14371

* Changelog: add #14386

* Changelog: add #14471

* Changelog: add #14325

* Changelog: add #14194

* Changelog: add #14340

* Changelog: add #14418

* Changelog: add #14417

* Changelog: add #14075

* Changelog: add #14467

* Changelog: add #14307

* Changelog: add #14326
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Eventbrite [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.

10 participants