PLANET-4810: Convert Media Block to WYSIWYG

[pablocubico]

Ref: https://jira.greenpeace.org/browse/PLANET-4810

I'm testing a "canonical save" approach, it uses the save() method as is intended by Wordpress.

There is a caveat to it: the save() method needs to be a pure function with no side effects or hooks, static content is saved in editing time, so we can't change the styles of a block saved using this method in the future. For example, any changes to MediaVideoElement will cause a validation error in the block editor.

So we have 3 options when rendering blocks:

1. Render in the client side using our frontendRendered, which stores a static DOM node as a placeholder, in which we inject the React component at run time on the frontend.
2. Render on the server side using Twig and ServerSideRender.
3. Save the content as static content using save() and a "pure" component.

Currently we are leaning towards (1), this makes perfect sense for heavily interactive blocks as the Spreadsheet. Pros: we can do a lot using React in the frontend. Cons: it may have some penalizations for SEO and no-script situations or JS errors.

This block uses (3), which is the "WP endorsed" way of dealing with static blocks. Pros: ideally, the content is stored in its "pure" form, with no side effects or after-render actions. But for example, in the case of MP4 files, this block adds some DOM manipulations after the initial render. This should only be used in cases where the content is absolutely static. Cons: we can't change the block once its saved. Any changes to the code generating the output will cause a validation error in the editor. We do those kind of changes sometimes, so maybe we don't what that resilience to change.

Method (2), the Twig templates: maybe this is not a bad option for some blocks, as it allows for server-side rendering dynamic blocks and integrates well with some WP flows. Pros: it's rendered on the server side and we have full control of what happens before the output, preventing any FOUC. We can change the way a block is rendered over time and it won't hurt the block in the editor. We can make retroactive changes to blocks inside content. Cons: we have to replicate the block in React to make it WYSIWYG in the Editor, duplicating the markup in JSX and Twig versions.

[mleray]

I think there is still some old code that needs to be removed:

• in test-shortcode-converter.php: here and here
• in class-shortcode-to-gutenberg.php: here

[mleray]

Looks pretty good 👍 I've added a few comments, mostly for small code clean-ups & improvements! Also the failing tests need to be fixed, by removing the old acceptance tests (+ create follow-up ticket to add JS acceptance tests) and fix some php spacing 🙂

[pablocubico]

@mleray comments addressed and rebased to master!

[mleray]

After testing this on local, it looks like existing blocks are not rendered at first and need to be re-saved! We could fix that by adding a render_callback as we did for other blocks 🙂

[pablocubico]

After testing this on local, it looks like existing blocks are not rendered at first and need to be re-saved! We could fix that by adding a render_callback as we did for other blocks 🙂

@mleray So I added the render_callback as in the other blocks but I just realized this is a static block, the output of save() is stored as is and the block is rendered statically, so it doesn't need it... I need to figure out what to do with existing content, hold on! 🤔

[Inwerpsel]

Having the frontend wait for php, which in turns waits for youtube's (or other) oembed endpoint response so that it finally can start performing requests for the embedded content, seems like something we probably want to avoid unless there is no other way, as it will be really slow.

We can have a look at the different embed types we offer and whether any of them has content that changes over time. If none of them do then saving it statically seems to make a lot more sense.

[pablocubico]

Having the frontend wait for php, which in turns waits for youtube's (or other) oembed endpoint response so that it finally can start performing requests for the embedded content, seems like something we probably want to avoid unless there is no other way, as it will be really slow.

We can have a look at the different embed types we offer and whether any of them has content that changes over time. If none of them do then saving it statically seems to make a lot more sense.

As said in one of the comments, the endpoint is only for old content, new content is statically saved and rendered directly here: https://github.com/greenpeace/planet4-plugin-gutenberg-blocks/pull/376/files#diff-ea623fe817268e0262e2bec08bdf6098R39

This is just to support old content without a migration, we can remove it if we migrate old Media blocks to static blocks in the near future.

[pablocubico]

@lithrel Indeed, I moved the video poster selector to the sidebar.
@Inwerpsel youtube_id deprecated & migrated, the MediaElementJS plugin is now loaded via useScript & useStylesheet.

[Inwerpsel]

Currently turning the youtube url into a nocookie one will not work anymore, definitely one to address. Also some things we can simplify a lot.

[pablocubico]

@Inwerpsel picking up from our latest conversations (you probably saw my comments via Toast notifications):

• undo/redo: it wasn't because of the state, it's happening on other blocks, I'll create a ticket for that
• wrapEmbedHTML: happening on the MediaFrontend block, not at save()-time nor in the PHP side
• other minor comments addressed

[Inwerpsel]

I'll finish the review tomorrow. Looks good so far, left some suggestions.

If we fallback with JS we probably want to run a migration soon as the difference with static html will probably be noticeable (assumption, still need to confirm the difference).

Thinking of that migration, it's actually going to be a bit tricky as we need to run JS to get the new block state, probably some setup involved in having that run on all posts 🤔

[Inwerpsel]

Looks good 👍 👍 👍

I recommend still making useScript and useStylesheet not retrying on error, it could result in the resource being added and removed on each render if the error persists. But probably not an issue for the usage in this PR 🤷

[mleray]

We need to remember to create a follow-up ticket to rewrite the acceptance tests in JS, but apart from that looks good to me 👍