-
Notifications
You must be signed in to change notification settings - Fork 18
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
Mobile: Add simple layout grid controls #187
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
const columnValues = {}; | ||
const numberOfColumns = selectedColumn.innerBlocks.length; | ||
for ( let pos = 0; pos < numberOfColumns; pos++ ) { | ||
for ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NP: Could we use forEach
or for of
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fixes now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only meant for the inner for loop, but this is fine 😅
In order for this PR to work as expected, we also need to refactor the @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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
Reserving this comment for manual testing with Gutenberg trunk/WP master Block error when inserting an image: image.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
*/ | ||
import { withDispatch } from '@wordpress/data'; | ||
|
||
export function withUpdateAlignment() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Good catch on accidental addition of |
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: 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. |
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 There is also a minor change to the The files that end with
It would be. The easier way would be to remove any changes to 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) 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. |
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. |
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.
After wordpress-mobile/gutenberg-mobile#3444 is merged we will be including the 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 |
This makes sure that the icon is the correct size on mobile.
78e1cbb
to
9244271
Compare
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). |
Thank you! |
const variations = [ | ||
{ | ||
name: 'one-column', | ||
title: __( 'One' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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.
To test on the web.
Run
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