Skip to content
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

Masonry: Fix heights init #3555

Merged
merged 2 commits into from
May 8, 2024

Conversation

liuyenwei
Copy link
Contributor

@liuyenwei liuyenwei commented May 7, 2024

Summary

This PR fixes an issue introduced by #3544 that was causing overlapping positions in the defaultTwoColumnModuleLayout when a multi column item was rendered towards the end of the first page.
image
image

The overlap happens in the situation when initializeHeightsArray is called and the following is true:

  • there is a two column item
  • the two column item is positioned before single column items that come after it in the array

i.e.
[A, B, C, D, E, F, G] - assuming E is the multi column item and the layout looks like:

| A | B | C |
| D | F | G |
| _ | E | E |

In this scenario, the current logic will think that the column height for column 1, 2 are B + E and C + E respectively.

Fix

This PR fixes this scenario by simplifying the logic. Ultimately, the height of each column is determine by the lowest position module in that column so we can change this to just be:
(for each item):

  • determine which column the item is positioned in
  • calculate its absolute height (top + height + gutter)
  • for each column the module spans, update the column height if the absolute height is greater

Test Plan

Tested this in pinboard - I managed to find an example that consistently repro'd
Also added unit tests around the heights logic using the same repro ^
image

@liuyenwei liuyenwei requested a review from a team as a code owner May 7, 2024 22:06
Copy link

netlify bot commented May 7, 2024

Deploy Preview for gestalt ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b213650
🔍 Latest deploy log https://app.netlify.com/sites/gestalt/deploys/663ab477e544d600089987e1
😎 Deploy Preview https://deploy-preview-3555--gestalt.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@liuyenwei liuyenwei changed the title [Masonry] Fix heights init Masonry: Fix heights init May 7, 2024
@liuyenwei liuyenwei added the patch release Patch release label May 7, 2024
@AlbertCarreras AlbertCarreras merged commit d9225d6 into pinterest:master May 8, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch release Patch release
Projects
None yet
3 participants