Mobile: Add simple layout grid controls

[enejb]

This Pr is the complete collection of the controls we want to ship as the first iteration of the Grid Layout block on mobile.

Related Gutenberg Mobile PR:
wordpress-mobile/gutenberg-mobile#3444

iOS PR:
wordpress-mobile/WordPress-iOS#16423

Android PR:
wordpress-mobile/WordPress-Android#14578

How has this been tested?

Test the full app via.

1. Open the App
2. Insert the Layout grid block.
3. Change the number of columns via the block settings.
4. Change the full-width alignment.
5. Change the vertical alignment. Each of the individual columns should have had the vertical alignment updated.
6. Add content to columns.
7. Change the padding of individual columns. Does the preview of the new padding look as expected?
8. Change the vertical alignment of the content. Does it work as expected?

To test on the web.
Run

yarn install
yarn build
yarn test

// remove the node_modules folder other wise the plugin is pretty big and might not install as expected
rm -rf node_modules

Create a zip of the block-experiments folder and install it as a plugin on your WordPress site.
Activate block experiments plugin that you just install.

Does the layout grid block still work as expected for you?
Are you able to switch between different column types.
Layout Grid:
Are you able to change Width, change vertical alignment, gutter sizes. different responsive columns?

Single column:
Able to set padding, able to set background colours?

On the web, you shouldn't notice any breaking changes.

Screenshots

Screen.Recording.2021-05-14.at.9.13.29.AM.mp4

[mkevins]

Nice work Enej! I tested this out, and it's mostly working as described. I had to install the layout grid block on the site first for the previews to work correctly. One thing I noticed (which may possibly be an issue with the preview flow, and not this PR) is that I added an image to the middle column of a group of three, but it does not appear in the preview. I'm not sure why, even after saving the post (and confirming it's contents via the HTML mode) that it still does not appear in the preview.

I left a few comments / questions / suggestions in the code.

[mkevins]

NP: Could we use forEach or for of here?

[enejb]

This should be fixes now.

[mkevins]

I only meant for the inner for loop, but this is fine 😅

[enejb]

In order for this PR to work as expected, we also need to refactor the isContainerRelated function see
WordPress/gutenberg#31625

@geriux and @mkevins can you take another look at this PR.

So far I have tested it locally on iOS and by running the web tests.
I tested on the iPad (in both orientations (portrait and landscape) and tried to get it to look as close to columns block as possible.

[geriux]

In order for this PR to work as expected, we also need to refactor the isContainerRelated

Thanks @enejb! I left a comment in that PR, let me know what you think.

Should we wait until the other PR is merged to test this one or should it work either way?

[mkevins]

The changes look good, and I like the refactor to share the code with web.

I tested this via the WordPress-Android app, but I wasn't able to build without the following changes there:

diff --git a/libs/editor/WordPressEditor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergPropsBuilder.kt b/libs/editor/WordPressEditor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergPropsBuilder.kt
index 330406e848..31fcef14f3 100644
--- a/libs/editor/WordPressEditor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergPropsBuilder.kt
+++ b/libs/editor/WordPressEditor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergPropsBuilder.kt
@@ -21,6 +21,7 @@ data class GutenbergPropsBuilder(
     private val editorTheme: Bundle?
 ) : Parcelable {
     fun build(activity: Activity, isHtmlModeEnabled: Boolean) = GutenbergProps(
+            enableLayoutGridBlock = true,
             enableContactInfoBlock = enableContactInfoBlock,
             enableMediaFilesCollectionBlocks = enableMediaFilesCollectionBlocks,
             enableMentions = enableMentions,

Also, I encountered some issues with the preview again. I'm not sure if this is related with the block, but I think it's worth mentioning, in case there is something going on that is related with the way we are using this block:

When I preview a post with the layout grid block, it displays as I expect, but subsequent previews are always stale. I even tried saving (which on the Android app leaves the editor) and re-opening the post. The weird thing is that the content is clearly saved, since I can see my changes in the editor, however, the preview still displays the older "stale" content.

Oddly, when I publish the post, and view it, it displays correctly, with the latest changes. But then I encountered a different issue: the preview of the published post attempts to open in the browser, which is not authenticated. This second issue doesn't feel related with your work here, so I'm only mentioning it since I encountered it as a result of the previous issue I found.

Here are some videos to demonstrate what I'm seeing.

[enejb]

Thanks for taking a look @mkevins

I wasn't able to replicate the issue that you faced and when I did there were some problems with any block that I tried. And the result wasn't related to the block as far as I could tell. I didn't dig much deeper into this thought.

Are you able to replicate this issue using a different block?

[mkevins]

Are you able to replicate this issue using a different block?

You are right! At first, I didn't see it with other blocks, but it happens after previewing more times. Aside from the preview issue (which seems unrelated to this PR) the block works as described.

The only change I think worth investigating is the need for so many additions to the lock-file. I'm wondering why it got so big, but I think that can be addressed separately (and maybe it is expected 🤔 ). Nice work Enej!

[gwwar]

Reserving this comment for manual testing with Gutenberg trunk/WP master

Block error when inserting an image:

image.mp4

[gwwar]

Did we mean to add package-lock.json in this PR? This looks like a new addition.

I did some quick smoke testing and I didn't see anything too out of sorts besides the image case I noted. I think it'd be good to get a +1 from @mkaz or others on Tinker that are a little more familiar with how the layout grid should work.

[gwwar]

I think it's clearer for the file name to be something like blocks/layout-grid/src/grid-column/hooks/with-update-alignment.js vs higher-order.js

[gwwar]

It's a bit late to catch this, but I think we've also been favoring hooks over HoC. Non-blocking, but see a non trivial example in https://github.com/WordPress/gutenberg/blob/09b32826b33d044f919fd9d90f86f7ff9cbb42c5/packages/block-editor/src/components/inserter/hooks/use-patterns-state.js

[enejb]

Thanks for the feedback @gwwar!

I think we should try to make the move hooks in a different PR, this one is pretty big as it is.
Thanks for the suggestion, will keep this in mind going forward.

[enejb]

Good catch on accidental addition of package-lock.json since it isn't needed.

[johngodley]

The unit tests all seem to work fine. I haven't done any manual testing yet, but will do so soon.

Can you explain more about what this PR is changing and why it's needed? It's quite a big PR so it's hard to dig in, and it mostly seems to be a refactor of grid code, with a few native files.

Would it be possible to split the core changes out from the native additions?

Also, just bringing it up as a FYI, but we do have a major refactor of the Layout Grid here:

#105

It's been on hold for quite some time so shouldn't be a concern for this PR, but it might help to understand the context for these changes and see whether #105 needs to be revisited.

[enejb]

Thanks for taking a look @johngodley!

This PR adds the mobile controls needed by the Mobile Gutenberg editor. To make the Layout Grid block work in the Mobile Gutenberg Editor. Here is the related Mobile Editor PR that adds the block to the editor ( wordpress-mobile/gutenberg-mobile#3444)

The main reason why we prioritized the layout grid block over some of the other blocks, for now, is that the block is used on the web side of things for many layouts on .com and they currently can't be easily edited in the Mobile Apps. We do have an unsupported block flow which is not ideal. Having this block available in the mobile editor would allow for sharing many different page layouts with the web as well.

Besides recreating some of the controls on the mobile editor side, in this PR we also refactored the web edit side of things so that we can share the same HOC components across both the web and the native implementation. The changes can be found in blocks/layout-grid/src/grid/higher-order.js and blocks/layout-grid/src/grid-column/hooks/with-update-alignment.js

There is also a minor change to the blocks/layout-grid/src/icons.js file that allows for the props.size to be passed in the block. To make the icon size work as expected on mobile.

The files that end with *.native.js or *.native.scss are mobile editor specific and only get used in the React Native build and shouldn't affect the way the web block is built.

Would it be possible to split the core changes out from the native additions?

It would be. The easier way would be to remove any changes to edit.js and icons.js files out of this PR and create a new PR with those changes but since these changes are more about the shared code implementation and not implementing any new features. It would be great to hear from the web team if this is the share code approach that we want to take, should we be implementing things differently? Not use HOC for example. I would rather keep the change in this one PR. Mostly to make development/testing easier. We end up with quite a few PRs on the mobile side of things for each PR on the block side of things.

The current layout grid block in the mobile apps only implements switching between different column sizes ( 1 column - 4 columns) for the columns block and for the column block, it only implements the "padding" control (small - huge)
some of the other controls we are planning to implement in the future based on the feedback from the users.

So if things change dramatically in #105 it might be easier to take those changes into account since not all the controls are implemented. The way that the block is saved and what attributes a block has is inherited from the web side of things. The save function is the same on both platforms.

Please let us know if you have any concerns and if you wish to make more changes.

[johngodley]

Thanks for the details!

We've discussed #105 internally and it is likely that we will finish it at some point. The changes there are substantial and do change what is output (in terms of HTML), but don't introduce any new features.

I don't think #105 will be ready soon though, so it probably makes sense to go with this PR if it's something you need soon.

I've given it a test and it all seems to work. If any problems are found we can sort them out in the release process.

Talking of which, I don't really know how this gets integrated in the mobile app. Do the native files need to be included in the plugin? Or is it sufficient that they just exist in Github, and you build it directly into the app?

Essentially I'm trying to work out if we need to make a new plugin release with these changes.

[enejb]

When the time comes and you are ready to merge #105 please let us know and we will be happy to make the changes so things are compatible with the mobile app side as well.

Talking of which, I don't really know how this gets integrated in the mobile app. Do the native files need to be included in the plugin? Or is it sufficient that they just exist in Github, and you build it directly into the app?

After wordpress-mobile/gutenberg-mobile#3444 is merged we will be including the https://github.com/Automattic/block-experiments/ repo as a submodule into the gutenberg-mobile repo.

From the mobile app perspective, there is no need to create a new plugin release for this change.

I think it would be interesting to investigate the possibility of bundling mobile app blocks with the plugin and releasing them to the .org repo. So that one day we can allow 3rd party plugin developers to create mobile app compatible blocks but that would be another project :)

Thanks for all the testing @johngodley

[johngodley]

I'm putting together a new version so I've merged this PR so it can be included in the final testing (on wp.com).

[enejb]

Thank you!

[fluiddot]

I'm working on adding i18n support for the Layout Grid block and I bumped into a case where these strings One, Two, Three, and Four are being missed because they don't reference the layout-grid domain as the rest of strings.

Not sure if it's an issue because I couldn't find the translations on either of both GlotPress projects (Gutenberg / Layout Grid) or it's expected to use the default domain, @enejb I'd appreciate it if you could provide further insights regarding this, thanks!

[enejb]

I think for one reason or another I thought that in Gutenberg we had the values translated. So I left them as they were taking the value from core. Since we didn’t translate blocks that were not living outside of Gutenberg. (.
I used the columns block as an example for this code. ) Or it might have been a typo and I missed adding the domain part of the function.

[fluiddot]

Great, thank you very much for info 🙇 !

I think for one reason or another I thought that in Gutenberg we had the values translated. So I left them as they were taking the value from core.

Yep, I was also surprised that number strings like "One" / "Two" aren't translated in Gutenberg, however, I noticed that we actually have them but with a dot at the end (i.e. "One.") 😅 .

Since we didn’t translate blocks that were not living outside of Gutenberg. (.

This is part of the work I'm doing for the Translation Pipeline Improvement project, although for the strings that are used only in the native version of the editor, they are currently being translated as part of the WordPress app.

I used the columns block as an example for this code. ) Or it might have been a typo and I missed adding the domain part of the function.

Ok, it's still unclear to me whether to use a domain or not within plugins, per this documentation about plugin translations, I understand that it's recommended to use a domain but I guess it's ok to fall back to default (Gutenberg in this case) if we know that the string is present there.

In any case, with the work I'm doing for the pipeline improvements, these strings will be added to the localization strings files and translated as strings of the WordPress app (example reference).