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: Bug fix, "basic" layout not left aligning #3558

Merged
merged 9 commits into from
May 16, 2024

Conversation

CalvinChhour
Copy link
Contributor

@CalvinChhour CalvinChhour commented May 9, 2024

Pull Request Template

Summary

Issue

When layout is set to basic, the docs state that the grid will be "Left-aligned, fixed-column-width masonry layout." This wasn't the case, due to gutterWidth pushing the items away from the left side. When gutterWidth is set to 0, it is appropriately left aligned, but then you have no gutter spacing between pins.

Additionally, center alignment was miscalculated due to the inclusion of an extra gutter (e.g. when you have 4 columns, you should only calculate 3 gutterWidths for the contentWidth)

What changed?

I've introduced a new optional prop justify which controls the horizontal alignment of items within the Masonry grid.
This change affects both Masonry and MasonryV2

Center Alignment

Center alignment was slightly off, due to an extra gutter being calculated

Before After
Kapture 2024-05-13 at 15 20 07 Kapture 2024-05-13 at 15 17 52

Start Alignment

We were center aligning before, we are now properly start aligning when justify='start' is set

Before After
Kapture 2024-05-13 at 15 20 07 Kapture 2024-05-13 at 15 22 06

End Alignment

End alignment didn't exist previously

Before After
Did not exist Kapture 2024-05-13 at 15 23 56

Why?

This change is needed for situations where we need the Masonry to be left aligned to match other Pin Grid formats (e.g. aligning Modules with PinReps with the Masonry).

This image highlights an issue we have, where the left aligned title looks disjointed from the Masonry grid.
image

Links

Checklist

  • Added unit and Flow Tests
  • Added documentation + accessibility tests
  • Verified accessibility: keyboard & screen reader interaction
  • Checked dark mode, responsiveness, and right-to-left support
  • Checked stakeholder feedback (e.g. Gestalt designers, relevant feature teams)

@CalvinChhour CalvinChhour requested a review from a team as a code owner May 9, 2024 21:46
@CalvinChhour CalvinChhour marked this pull request as draft May 9, 2024 21:49
Copy link

netlify bot commented May 9, 2024

Deploy Preview for gestalt ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 4da793e
🔍 Latest deploy log https://app.netlify.com/sites/gestalt/deploys/66464e6f0c18a70009aafd02
😎 Deploy Preview https://deploy-preview-3558--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.

@CalvinChhour CalvinChhour changed the title {Masonry} Bugfix for masonry "basic" layout not left aligning Masonry Bug fix, "basic" layout not left aligning May 9, 2024
@CalvinChhour CalvinChhour changed the title Masonry Bug fix, "basic" layout not left aligning Masonry: Bug fix, "basic" layout not left aligning May 9, 2024
@CalvinChhour CalvinChhour marked this pull request as ready for review May 9, 2024 22:11
Copy link
Contributor

@liuyenwei liuyenwei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a matter of the prop (and documentation) being confusing. We currently assume that Masonry is always "centered" within the container. What the justify prop does is control where items start lining up from (#929 is perhaps a more clear example of this). The confusing bit is that this only impacts the situation where a grid does not contain enough items to fill up the container width.

image
vs
image

This PR as-is would break quite a few things since it would cause all grids on pinterest to shift left
image

I think one way we can solve this is to introduce justify prop to Masonry, something like justify: start|center|end which we can pass in to all layouts and control whether the grid is actually rendered left, center, or right-aligned. For basicCentered - I think we can leave as-is for now, and revisit separately

@jackhsu978
Copy link
Member

@CalvinChhour sorry for the short notice, but I am planning on landing #3567 this week for TypeScript migration, and this PR will conflict with that. Can you check with @liuyenwei and determine if you'll be able to merge this PR tomorrow (5/16)? If so, I will wait and apply the same changes to the TS branch.

Copy link
Contributor

@liuyenwei liuyenwei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one comment about keeping the wording consistent between align vs justify but otherwise lgtm. thank you for making the changes!

packages/gestalt/src/Masonry/defaultLayout.js Outdated Show resolved Hide resolved
@AlbertCarreras AlbertCarreras added the minor release Minor release label May 16, 2024
@AlbertCarreras AlbertCarreras merged commit cf71d97 into pinterest:master May 16, 2024
13 checks passed
@CalvinChhour CalvinChhour deleted the patch-1 branch May 16, 2024 19:01
jackhsu978 added a commit to jackhsu978/gestalt that referenced this pull request May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor release Minor release
Projects
None yet
4 participants