Move PictureLayout to grid - #16251
Conversation
f33830a to
e33629d
Compare
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
e2f5162 to
d9fba56
Compare
6729bd2 to
53aad3f
Compare
JamieB-gu
left a comment
There was a problem hiding this comment.
Overall approach looks good. Comments mostly about application of CSS and the grid rules.
| headline: { | ||
| mobile: 'grid-row: 2;', | ||
| tablet: 'grid-row: 2;', | ||
| desktop: `grid-row: 2; ${grid.between('centre-column-start', 'right-column-end')};`, |
There was a problem hiding this comment.
I think you're asking this element to span both columns here, then on the element inside it you're setting a max-width to ensure it's essentially the same width as the centre column? Would it be easier to just set grid.column.centre here (which I think is the default) and remove that max-width, or am I missing something about the design?
9877fdf to
ebfdf4a
Compare
|
Please rebase this branch against
Please rebase this branch against |
5cab1a2 to
bbe6b1b
Compare
Co-authored-by: Jamie B <53781962+JamieB-gu@users.noreply.github.com>
Co-authored-by: Jamie B <53781962+JamieB-gu@users.noreply.github.com>
Co-authored-by: Jamie B <53781962+JamieB-gu@users.noreply.github.com>
c75a9d3 to
cc995bc
Compare
fbd642f to
b92684d
Compare
2140727 to
2b18bb3
Compare
2b18bb3 to
7e10e7c
Compare
| id="maincontent" | ||
| css={[ | ||
| `margin-top: ${remSpace[3]}`, | ||
| `margin-top: ${format.design === ArticleDesign.Picture ? 0 : remSpace[3]}`, |
There was a problem hiding this comment.
This keeps parity with the Chromatic tests though I'd argue it's better for this to be consistent





Continuing on my grid rampage of #1542, #16119, and #16133 this absorbs PictureLayout into the new grid system for content pages. This one's a smidge more involved due to the avatar pics that sometimes render (though cracking that here paves the way for CommentLayout to move over too).
Screenshots