[Mobile] - Refactor gallery - cherry pick Grid component #32227
Conversation
Note: Since render was changed to renderContent, we should return early from render too, when blockWidth is falsey.
I'm not sure yet whether it still makes sense to use blockProps in this way. I'm going to cherry-pick the commits by file, and sort out the need for this mechanism afterwards.
Since this is duplicated from the original mobile gallery code (Tiles component), it might make sense to export it for re-use. Previously, it was only moved, but now that we will maintain both versions, it has become a duplicate implementation. I will defer this to a later commit.
Similar to blockProps mentioned earlier, gridProperties will be evaluated after cherry-picking the relevant changes, to see if there is another mechanism that may be more appropriate.
Note: This also re-adds fullWidth style, which is still being used in both v1 and v2 mobile implementations. If this is superceded by a recent refactor of the block width styles, it may be worth revisiting this and removing / changing the implementation.
Note: as before, blockProps and gridProperties should be re-evaluated in subsequent commits, if necessary. E.g. `imageCrop` is already recieved via context, and `isGroup` may be sufficient, eliminating the need for `isGallery`.
Going back over the older commits, it seems there were a two strategies used to render the gallery images as a grid. One used the numColumns (same as used in the Columns block), and the other using the Grid component. This commit cleans up the unused parts of the former approach.
This is no longer necessary, since we are using context to pass gallery-level attributes to the image blocks' rendering.
mkevins
added
Mobile App - i.e. Android or iOS
There was a problem hiding this comment.
Great work @mkevins 👍
I tested this on a Pixel 5 (Android 12) and an iPhone SE 2020 (iOS 14.4.1) with the WPAndroid Apps using metro. Everything works as described and the code looks good 🎉
I'm noting some small issue below but I think we can proceed with merging this in the main mobile refactor branch :)
though still not as performant as former implementation
The performance seems degraded especially when you add multiple gallery blocks. I think it's not unusably slow thought and we can tackle it in a next iteration.
I noticed two pretty minor issue and noting them here just to keep track:
- The old gallery block (at its more recent state) opens the Choose images bottom sheet when a new gallery block is added. This behavior does not persist in the new gallery block.
- In landscape mode with 4 columns the rearrange arrows are not visible.
Screenshots
| iPhone SE 2020 | Pixel 5 |
|---|---|
 |
 |
|
Thanks for reviewing and testing this out Antonis! Also, thanks for noting those additional issues. I'll copy them to the main mobile PR, in a known issues section. I also noticed an issue that may be worth addressing in a subsequent PR, which is that when the gallery is inside a column (where there are two columns side-by-side) on landscape, the available width is reduced, but the |
mkevins
deleted the
rnmobile/refactor/gallery-as-nested-image-blocks-cherry-pick-grid
branch
Related PRs
Description
This PR brings the changes from the former PR which adds a
Gridcomponent to layout the gallery images, rather than relying on thenumColumns. This seems to be a bit more performant, though still not as performant as former implementation (noted here), which did not rely uponInnerBlocks.Screen recordings
before-numCols-to-grid.mp4
after-numCols-to-grid.mp4
How has this been tested?
Create a gallery in the editor on the main app and add some images (at least 5). Change the columns setting in the gallery block to different values in portrait and landscape. The images should be displayed in 2 columns maximum on portrait, and 4 columns maximum on landscape, or the number of columns based on the setting, if it is less than those maximums.
Also, try moving the images around within the gallery. The performance should be improved compared to the previous state of the refactored implementation.
Checklist:
*.native.jsfiles for terms that need renaming or removal).