-
Notifications
You must be signed in to change notification settings - Fork 337
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
Conversation
✅ Deploy Preview for gestalt ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
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 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.
This PR as-is would break quite a few things since it would cause all grids on pinterest to shift left
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
@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. |
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.
one comment about keeping the wording consistent between align vs justify but otherwise lgtm. thank you for making the changes!
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 togutterWidth
pushing the items away from the left side. WhengutterWidth
is set to0
, 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 thecontentWidth
)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 calculatedStart Alignment
We were center aligning before, we are now properly start aligning when
justify='start'
is setEnd Alignment
End alignment didn't exist previously
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.
Links
Checklist