Closed jorostoyanov closed 1 month ago
Great work! Thank you. I tested this and it works like a charm.
There is a tiny bit of code standard stuff like missing spaces between parenthesis and vars in the for loops. I would maybe also prefer it if we could use .filter()
on blocks
and newBlocks
instead of continue in for loops, but that is not something you have to do if you disagree.
With this I also think we can get rid of a one of the PHP patchers. I tested it and without this class here: https://github.com/Automattic/newspack-content-converter/blob/a886de47e19f7019e601572b764eb7a9fa3b4350/lib/content-patcher/patchers/class-shortcodepreconversionpatcher.php#L43 and it seems to only add some newlines, so I think we can remove it. What do you think? Should we do it in this PR or a new one?
And then one last question that might be way out of scope for this PR: We tend to prefer using the Jetpack Gallery Block over the vanilla one. – It would be cool to use that instead of the vanilla gallery. I don't know that we can just decide to use that because it would break for people not using Jetpack, but let's think about how we could allow for both to be an option maybe?
Hey @naxoc 👋
Thank you for the feedback. It all makes sense 🙏
I've updated the code to match the coding standards you've mentioned.
Regarding the PHP Patcher — I'll do it in a separate PR, if you don't mind. But yes, you are right, it appears that Gutenberg does the newlines out-of-the-box now and the shortcode patcher is not needed anymore :)
Regarding the Jetpack Gallery Block — I feel like we need to discuss this in more details and based on that proceed with the change. I will open the discussion for that and continue from there.
I would be thankful if you can have a final look or react that this can be merged now 🙏 🙇
Related Issue: https://github.com/Automattic/newspack-content-converter/issues/140
Add support for Galleries conversion from a shortcode to a Gallery Block.
The approach that we followed prior to this PR was not converting the Gallery shortcode to proper Gallery Block. Instead there were several different issues.
1) When there is a [gallery ids="24,25,26"] only in a Post Content
Imagine the following Post Content:
The shortcode was converted to a Paragraph Block with the shortcode inside in the following format:
The reason behind that was that the default behaviour in WordPress is to always wrap content in paragraphs.
While debugging this, I've found that when using the following code to parse the Classic Content into a Classic Block (core/freeform) it doesn't get it wrapped in paragraphs:
So, instead of doing this I've followed how Gutenberg does it when hydrating the initial Post Content in the editor and found that they are using the
parse
function in the following way:This, wrapped the Gallery shortcode in a paragraph internally and converting the content led to a gallery block with empty images.
2) When there is a paragraph with a gallery shortcode inside
Example content:
When converting to Blocks, the output contained a skeleton of a Gallery with the proper number of inner blocks for images, but didn't show any images.
Example output:
Notice that the
<img>
tags don't contain anysrc
attribute and thus the image is not displayed!After some further investigation, I found that Gutenberg does this by default. In order to populate the needed img src attributes, there is an additional step that queries the attachments data from the REST API and uses the response to populate the missing Gallery details.
This approach makes sense since we don't have the Attachment details initially (remember that the Gallery shortcode contains only the IDs of the Attachments).
So, naturally this PR adds the missing resolution of Gallery Images and changes the inner blocks to contain the needed details.
In addition to that, with this PR we are preserving the caption of the images, which currently is broken when using the Gutenberg Convert to Blocks button (PR for the Gutenberg Repo soon to be opened 🤞)
Finally, the proper end result of a converted post containing Gallery Shortcode will now be: