[RNMobile] Gallery - Tiles component

[mkevins]

Description

This PR introduces a Tiles component for the semi-cross-platform Gallery block. The purpose of this component is to introduce a responsive layout abstraction for mobile views.

PR Hierarchy

This PR is part of the PR hierarchy described here. This PR can be tested with the aggregate changeset and integration of components within the related PRs by checking out the branch of the "top level" PR: #18265.

To test

Test this component by checking out the branch of the "top level" PR: #18265.

Open the gallery settings from the demo app and change the "Columns" value (in the BottomSheet) to different values.

Expected behavior in portrait

• Tiles layout should update to reflect the columns setting (visible columns should not exceed 2, but will go down to 1)
• Tiles should be evenly spaced
• Tiles in last row should expand to fill available width

Expected behavior in landscape

• Tiles layout should update to reflect the columns setting
• Tiles should be evenly spaced
• Tiles in last row should expand to fill available width

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[koke]

This one looked like a typo until I decided to check the dictionary first (TIL 😁). I think spacing works as well and might be clearer/shorter.

[mkevins]

Perhaps I took the metaphor a bit too far 😆. I changed this to spacing.

[pinarol]

I agree that spacing work better. I should say though, when I found out what grout means I had a lol moment, I appreciate how creative this metaphor is :D

[koke]

Why are we setting these as borders and not margins?

[mkevins]

I used borders for this so that fixed sizes would be included in the total width of each item, preserving the correct wrapping behavior. I've added a note about this in the code comment.

[koke]

I didn't realize I had said margins here when I finished the review. Why not paddings? I would think they offer the same effect but they would be more meaningful than transparent borders

[mkevins]

I think padding is semantically better than border for what we are doing here, and I even used padding in an earlier iteration (from what I can tell it works pretty much the same way). The reason I switched back to using border is that I had the thought that border would be less likely to be used later if we wanted to modify the styles, so it was more "disposable" in that sense (i.e. by not using padding, we still have the opportunity to use it for its original purpose, if that need arises).

[pinarol]

(i.e. by not using padding, we still have the opportunity to use it for its original purpose, if that need arises).

Could you help me understand this better? What kind of need for example? Using borders for spacing purposes is not very intuitive for other people who will maintain it. And having the need to put such a big code comment can be signaling that we are introducing a "hard to understand" code. So I'd prefer using paddings or margins unless we have good reasons.

[pinarol]

It looks like I made a mistake on this line:

const isRightMostTile = ( index === lastTile ) || (( index + 1 ) % rowLength === 0);

The correct one should be:

const isRightMostTile = ( index === lastTile ) || (( index + 1 ) % columns === 0);

It is working fine with this fix according to my experiments.

BTW I am fine with both:

• going with percentages and using paddings
  or
• going with margins and calculating the width in pixels

I just wouldn't prefer to go with transparent borders as it is not what they are for and for that reason it is making code harder to understand. And we can always iterate if a special need comes up that will require us to use borders for spacing.

When it comes to paddings vs margins, they have both pros/cons. Advantage of padding solution is we don't need the onLayout handling.

Advantage of margin solution is, it is making the space calculation very easy, the only thing to calculate is left as: marginRight: isRightMostTile ? 0 : spacing,.

Personally I'd go with margins but I am fine with both solutions. 👍

I wouldn't expect any performance differences between both ways because whenever container width changes the tiles should be re-rendered whether they are calculated in pixels or percentages.

[mkevins]

Hi @pinarol 👋

The correct one should be:

const isRightMostTile = ( index === lastTile ) || (( index + 1 ) % columns === 0);

I've tried the margin approach again with these suggested changes, however, I'm still seeing the same layout problem on orientation changes described previously.

BTW I am fine with both:

• going with percentages and using paddings
  or
• going with margins and calculating the width in pixels

I just wouldn't prefer to go with transparent borders as it is not what they are for and for that reason it is making code harder to understand. And we can always iterate if a special need comes up that will require us to use borders for spacing.

Unfortunately, neither border nor padding are semantically correct in how they're being used here, according to the box model. I don't have a strong opinion about which is "better" between border and padding, so I've changed it to padding.

When it comes to paddings vs margins, they have both pros/cons. Advantage of padding solution is we don't need the onLayout handling.

Advantage of margin solution is, it is making the space calculation very easy, the only thing to calculate is left as: marginRight: isRightMostTile ? 0 : spacing,.

Personally I'd go with margins but I am fine with both solutions.

I wouldn't expect any performance differences between both ways because whenever container width changes the tiles should be re-rendered whether they are calculated in pixels or percentages.

Another advantage to the current spacing interpolation approach is that it allows Tiles to remain a stateless functional component instead of a stateful class component.

Since the onLayout approach seems to result in an unstable layout during orientation changes, I've gone with the interpolation approach. I've pushed a change to use padding instead of border, and updated the comment to reflect that.

[pinarol]

I've tried the margin approach again with these suggested changes, however, I'm still seeing the same layout problem on orientation changes described previously.

I am super curious why that's happening now because I was able to repro the problem and that change seemed fixing that. If you want we can work on the problem together. But as I said before I am also fine with using paddings.

Unfortunately, neither border nor padding are semantically correct in how they're being used here

You have a point, but I wouldn't consider paddings and borders as equally incorrect if they are used for spacing purposes. And if we are looking for semantically the most correct one that seems to be margins.

Since the onLayout approach seems to result in an unstable layout during orientation changes, I've gone with the interpolation approach.

If you mean this problem by unstable layout I think that's not because of onLayout, that one is just a result of an immature solution(I spent only ~30mins) and the logic that calculates the right most tile can have flaws, it just needs some tackling. Or is there any other problem that I am missing?

[mkevins]

Or is there any other problem that I am missing?

I'm not sure. fwiw, your logic to determine the right-most tile looks correct to me. In fact, I had earlier implementations that used a similar approach with margin, and even flex-basis, but all of them suffered from layout problems on orientation changes. The only solution that seems robust in my tests is the current interpolation approach. When using the onLayout approach, adding console.log(width) showed some differences in the layout calculations:

Starting app in landscape:

2x 358.5454406738281 tiles.native.js:35
5x 360.7272644042969 tiles.native.js:35

After orientation change:

3x 548               tiles.native.js:35
3x 546.1818237304688 tiles.native.js:35

For each, onLayout is called 7 and 6 times, respectively, for portrait and landscape orientations. It seems that the width "adjusts" as the calculation "settles" on a final result. In the case of portrait, width seems to grow slightly, whereas in landscape it seems to shrink.

[pinarol]

I am all fine with going with padding solution 👍 onLayout can be called multiple times but we don't need to update the component state if container width stays same. And if container width does change, the tiles will need layout again independent from whether they are calculated using pixels or percentages.

But if you observed some odd behavior in case of margins let's go with paddings.

I am up for both: going as is or working together to see what's wrong with margin solution(because I couldn't repro with my local env).

[koke]

This is looking pretty solid. I found some discrepancies on image sizing, but I think those are more related to GalleryImage and not the Tiles component.

The logic to distribute the spaces evenly was not obvious from the start. I had to run it through the debugger and simulate a few scenarios before I realized what was going on. It is also different from what the web does. I went to suggest simpler alternatives, but then I realized that without a native alternative to calc(), this was our best way to ensure equal widths and spacing for all images.
So the implementation is good, but still hard to follow, and I think it could benefit from some comments explaining what it's trying to do and why.

Interestingly enough, I tested in RTL mode and react-native seems to automatically flip those border widths correctly 😮

[mkevins]

The logic to distribute the spaces evenly was not obvious from the start. I had to run it through the debugger and simulate a few scenarios before I realized what was going on. It is also different from what the web does. I went to suggest simpler alternatives, but then I realized that without a native alternative to calc(), this was our best way to ensure equal widths and spacing for all images.
So the implementation is good, but still hard to follow, and I think it could benefit from some comments explaining what it's trying to do and why.

I agree, and I've added an explanatory code comment for the layout logic used here.

[koke]

Thanks for the great explanation

[mkevins]

Thanks for reviewing Koke!

FYI: Just added an empty commit to trigger travis to re-run tests (the re-run button seemingly isn't working).