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

Blocks: Add a new Google Calendar block #13999

Merged
merged 27 commits into from
Feb 20, 2020
Merged

Conversation

pento
Copy link
Contributor

@pento pento commented Nov 8, 2019

This is the first step towards replacing the [googleapps] shortcode with blocks.

This PR is currently a work in progress, so please don't be surprised when things don't work. 🙂

Things that aren't working yet:

Changes proposed in this Pull Request:

Add a new block, jetpack/google-calendar.
Add a block version gating arg to jetpack_register_block

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.

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.

This block also introduces version gating, and you will need wp > 5.3 or gutenberg plugin >= 7.2 to see the block.

  • Open the Block Editor.
  • Try pasting Google Calendar embed URLs or iframe as an admin user: https://calendar.google.com/calendar/embed?src=jb4bu80jirp0u11a6niie21pp4%40group.calendar.google.com&ctz=America/New_York
    or
    <iframe src="https://calendar.google.com/calendar/embed?src=jb4bu80jirp0u11a6niie21pp4%40group.calendar.google.com&ctz=America/New_York" style="border: 0" width="800" height="600" frameborder="0" scrolling="no"></iframe>
  • Try editing the embed to a different URL.
  • Try converting Classic posts with the Google Calendar <iframe> in them.
  • Try converting Classic posts with a Google Apps short code in them: [googleapps domain="calendar" dir="calendar/embed" query="src=jb4bu80jirp0u11a6niie21pp4%40group.calendar.google.com&ctz=America/New_York" width="800" height="600" /]
  • Try accessing the block on WP < 5.4 or gutenberg plugin < 7.2 - it shouldn't be available in versions less than these

gcal2

Proposed changelog entry for your changes:

  • Blocks: Add a new Google Calendar block.

@pento pento added [Status] In Progress [Type] Feature Request [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Nov 8, 2019
@pento pento added this to the 8.0 milestone Nov 8, 2019
@pento pento requested a review from a team as a code owner November 8, 2019 06:10
@pento pento self-assigned this Nov 8, 2019
@jetpackbot
Copy link

jetpackbot commented Nov 8, 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: March 3, 2020.
Scheduled code freeze: February 25, 2020

Generated by 🚫 dangerJS against fa55f5e

@jeherve
Copy link
Member

jeherve commented Nov 8, 2019

replacing the [googleapps] shortcode
Add a new block, jetpack/google-calendar.

I may be missing something obvious, but should the block be called differently if it aims to embeds different kinds of Google Content and not just calendars? Could it be "Google Content", "Google Embed", "Google services" instead?

In any case, it may be useful to consider the AMP plugin if we develop any kind of Google-related block. Googlers have been contributing quite a bit of enhancements to Jetpack in the past few months to help make each one of our features compatible with AMP views. I think it would be nice if this new block supported that as well.

Here is a recent example with the Slideshow block: #13009

@pento
Copy link
Contributor Author

pento commented Nov 12, 2019

On the topic of a single "Google Suite" block, vs individual blocks for different Google services, @davemart-in has mentioned a preference for individual blocks. I suspect that there'll be room for different options on each block depending on which service is being embedded.

👍🏻 on adding AMP support.

While investigating transforms, I've run into WordPress/gutenberg#10674, which will prevent transforming from the [googleapps] shortcode, since it needs to determine which kind of Google service the shortcode is transforming from. WordPress/gutenberg#8569 is also kind of a problem, for that matter, we have to manually provide a transformation for [googleapps] shortcodes that have been previously transformed into the core/shortcode block.

@pento
Copy link
Contributor Author

pento commented Nov 12, 2019

Given the issues in Gutenberg I've run into here, and with Jetpack supporting the current and previous versions of WordPress, I'm wondering if it's worth exploring version gating this block. If the Gutenberg issues are fixed, we can say the block is only available if you're running Gutenberg x.y+, or WordPress 5.4+.

Does Jetpack have any examples of calls to action encouraging people to upgrade their WordPress version (or install another plugin) to get full functionality?

@jeherve
Copy link
Member

jeherve commented Nov 12, 2019

If the Gutenberg issues are fixed, we can say the block is only available if you're running Gutenberg x.y+, or WordPress 5.4+.

We have had pieces of code that were previously only available for specific WordPress versions (the Jetpack tests inside the new Tools > Site Health screen come to mind: https://github.com/Automattic/jetpack/blob/7.8/_inc/lib/debugger/0-load.php#L19 )

Similarly, some of the things we offer require a specific plugin. The whole 3rd-party directory relies on specific plugins, and does that by hooking into specific filters, checking for classes, or checking for functions. In some cases, we check for specific functions to catch a specific plugin version:

* Wrap in function exists check since this requires WooCommerce 3.3+.

I think we could do the same when we register that block, that should be fine. That will impact the number of sites where the feature is available, but you will be okay with Atomic sites though.

Does Jetpack have any examples of calls to action encouraging people to upgrade their WordPress version (or install another plugin) to get full functionality?

We don't really have that in Jetpack today. You could, however, create a JITM that would be displayed to a specific subset of users to invite them to install Gutenberg for example.

@kraftbj
Copy link
Contributor

kraftbj commented Nov 12, 2019

With a conditional, please do add an inline todo comment explaining when the check can be removed ( like https://github.com/Automattic/jetpack/pull/13980/files#diff-dd77455a3e202f94f73c1963586b01e3L232 that was just removed). Sometimes it is obvious (a function_exists check for a new 5.4 WP function), but just to be safe so future us are confident that we're okay to remove older stuff.

@pento
Copy link
Contributor Author

pento commented Nov 18, 2019

As this is going to require a bit more foundational work, I'm moving it out of the 8.0 milestone for now.

@pento
Copy link
Contributor Author

pento commented Feb 3, 2020

I've just been reviewing where things are at in the blockers for this PR.

Completed

In Progress

Stalled/Not Started

@matticbot
Copy link
Contributor

pento, Your synced wpcom patch D38733-code has been updated.

@pablinos pablinos mentioned this pull request Feb 17, 2020
@davemart-in
Copy link
Contributor

@davemart-in, are you happy with the block preview instructions moved to an

    which is more semantically correct, and will make the translation simpler, but doesn't have the Step) wording from the designs as below:

@glendaviesnz yes that is fine. I like that approach even better. Nice adjustment! Thanks.

<>
<InspectorControls>
<PanelBody title={ __( 'Calendar Settings', 'jetpack' ) } initialOpen={ false }>
{ this.getEditForm( `${ className }-embed-form-sidebar`, editedEmbed ) }
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 causing issues when you use the custom css class field in the editor:

image
image

This is a problem a few lines down as well.

Related: #14623

Copy link
Contributor

Choose a reason for hiding this comment

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

Added a fix for this, and let @pablinos know re #14623, will see if he thinks there is a better way to fix this or not.

extensions/blocks/google-calendar/index.js Outdated Show resolved Hide resolved
@matticbot
Copy link
Contributor

pento, Your synced wpcom patch D38733-code has been updated.

@glendaviesnz
Copy link
Contributor

@jeherve - I think all the latest feedback has been covered off, so it may be good to go.

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. 👍

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! 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 Feb 20, 2020
@matticbot
Copy link
Contributor

pento, Your synced wpcom patch D38733-code has been updated.

@glendaviesnz glendaviesnz merged commit 926bdd1 into master Feb 20, 2020
@glendaviesnz glendaviesnz deleted the add/google-calendar-block branch February 20, 2020 21:57
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Feb 20, 2020
@scruffian
Copy link
Member

r203268-wpcom

jeherve added a commit that referenced this pull request Feb 25, 2020
jeherve added a commit that referenced this pull request Feb 25, 2020
* 8.3 release: changelog

* Changelog: add #14516

* Changelog: add #14574

* Bring in changes from 8.2.1 and 8.2.2

* Update stable version

* Bring in 8.2.3 changes

* Changelog: add #14714

* Changelog: add #14639

* Changelog: add #14678

* Changelog: add #14673

* Changelog: add #14687

* Changelog: add #14704

* Changelog: add #14702

* Changelog: add #14541

* Changelog: add #14657

* Changelog: add #14622

* Changelog: add #14582

* Changelog: add #14638

* Changelog: add #14633

* Changelog: add #14571

* Changelog: add #14592

* Changelog: add #14539

* Changelog: add #14514

* Changelog: add #14643

* Changelog: add #14494

* Changelog: add #13739

* Changelog: add #14707

* Changelog: add #14736

* Changelog: add #14706

* Changelog: add #14730

* Changelog: add #14685

* Changelog: add #14727

* Changelog: add #14711

* Changelog: add #14742

* Changelog: add #14746

* Changelog: add #14725

* Changelog: add #13999

* Changelog: add #14740

* Changelog: add #14759

* Changelog: add #14703

* Changelog: add #14753

* Changelog: add #14754

* Changelog: add #14645

* Cahngelog: add #14599
@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 and confirm D59607-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@jeherve
Copy link
Member

jeherve commented Apr 1, 2021

^ The above seems to have been created by mistake. Abandoning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Google Calendar [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.