Skip to content

Move PictureLayout to grid - #16251

Open
frederickobrien wants to merge 11 commits into
mainfrom
move-picture-layout-into-grid
Open

Move PictureLayout to grid#16251
frederickobrien wants to merge 11 commits into
mainfrom
move-picture-layout-into-grid

Conversation

@frederickobrien

@frederickobrien frederickobrien commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

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

Before After
before after
before2 after2
before3 after3

@frederickobrien frederickobrien self-assigned this Jun 23, 2026
@frederickobrien frederickobrien added the maintenance Departmental tracking: maintenance work, not a fix or a feature label Jun 23, 2026
@frederickobrien frederickobrien added this to the Interactives milestone Jun 23, 2026
@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown

@frederickobrien frederickobrien changed the title Absorb PictureLayout into grid Move PictureLayout to grid Jun 24, 2026
@frederickobrien frederickobrien added the run_chromatic Runs chromatic when label is applied label Jun 24, 2026
@github-actions github-actions Bot removed the run_chromatic Runs chromatic when label is applied label Jun 24, 2026
@frederickobrien
frederickobrien marked this pull request as ready for review June 24, 2026 13:03
@frederickobrien
frederickobrien force-pushed the move-picture-layout-into-grid branch from f33830a to e33629d Compare June 24, 2026 13:03
@github-actions

Copy link
Copy Markdown

Hello 👋! When you're ready to run Chromatic, please apply the run_chromatic label to this PR.

You will need to reapply the label each time you want to run Chromatic.

Click here to see the Chromatic project.

@frederickobrien
frederickobrien requested a review from a team June 24, 2026 13:03
@frederickobrien frederickobrien added the run_chromatic Runs chromatic when label is applied label Jun 24, 2026
@github-actions github-actions Bot removed the run_chromatic Runs chromatic when label is applied label Jun 24, 2026
@frederickobrien
frederickobrien force-pushed the move-picture-layout-into-grid branch 2 times, most recently from e2f5162 to d9fba56 Compare June 30, 2026 09:50
@frederickobrien frederickobrien added the run_chromatic Runs chromatic when label is applied label Jun 30, 2026
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown

@github-actions github-actions Bot removed the run_chromatic Runs chromatic when label is applied label Jun 30, 2026
@frederickobrien
frederickobrien force-pushed the move-picture-layout-into-grid branch 2 times, most recently from 6729bd2 to 53aad3f Compare July 3, 2026 09:14

@JamieB-gu JamieB-gu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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')};`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's my attempt to handle the existing appearance where the grid section as a whole spans all the way to the end of the right column, but the headline itself doesn't. E.g.

image

Comment thread dotcom-rendering/src/layouts/lib/articleArrangements.ts Outdated
Comment thread dotcom-rendering/src/layouts/lib/articleArrangements.ts Outdated
Comment thread dotcom-rendering/src/layouts/lib/articleArrangements.ts
Comment thread dotcom-rendering/src/layouts/lib/articleArrangements.ts Outdated
Comment thread dotcom-rendering/src/layouts/lib/articleArrangements.ts Outdated
Comment thread dotcom-rendering/src/layouts/lib/articleArrangements.ts Outdated
Comment thread dotcom-rendering/src/layouts/lib/articleArrangements.ts Outdated
Comment thread dotcom-rendering/src/layouts/lib/articleArrangements.ts Outdated
Comment thread dotcom-rendering/src/layouts/lib/articleArrangements.ts Outdated
@frederickobrien
frederickobrien force-pushed the move-picture-layout-into-grid branch 2 times, most recently from 9877fdf to ebfdf4a Compare July 9, 2026 13:25
@akash1810

akash1810 commented Jul 9, 2026

Copy link
Copy Markdown
Member

Please rebase this branch against main before deploying to CODE. #16321 made some changes to CI and infrastructure. Deploying this branch without these changes present will either:

  • Fail when using Riff-Raff's default update strategy
  • OR delete the new infrastructure if using Riff-Raff's "dangerous" mode

Please rebase this branch against main before deploying to CODE.

@frederickobrien
frederickobrien force-pushed the move-picture-layout-into-grid branch 2 times, most recently from 5cab1a2 to bbe6b1b Compare July 22, 2026 12:04
@frederickobrien frederickobrien added the run_chromatic Runs chromatic when label is applied label Jul 22, 2026
@github-actions github-actions Bot removed the run_chromatic Runs chromatic when label is applied label Jul 22, 2026
@Jakeii

Jakeii commented Aug 12, 2026

Copy link
Copy Markdown
Member

There appear to be some unintentional differences now:
This PR:
Screenshot 2026-08-12 at 09 39 14

prod:
Screenshot 2026-08-12 at 09 39 21

@frederickobrien
frederickobrien force-pushed the move-picture-layout-into-grid branch from c75a9d3 to cc995bc Compare August 19, 2026 11:50
@frederickobrien frederickobrien added the run_chromatic Runs chromatic when label is applied label Aug 19, 2026
@github-actions github-actions Bot removed the run_chromatic Runs chromatic when label is applied label Aug 19, 2026
@frederickobrien frederickobrien added the run_chromatic Runs chromatic when label is applied label Aug 19, 2026
@github-actions github-actions Bot removed the run_chromatic Runs chromatic when label is applied label Aug 19, 2026
@frederickobrien
frederickobrien force-pushed the move-picture-layout-into-grid branch from fbd642f to b92684d Compare August 19, 2026 12:30
@frederickobrien frederickobrien added the run_chromatic Runs chromatic when label is applied label Aug 19, 2026
@github-actions github-actions Bot removed the run_chromatic Runs chromatic when label is applied label Aug 19, 2026
@frederickobrien
frederickobrien force-pushed the move-picture-layout-into-grid branch 2 times, most recently from 2140727 to 2b18bb3 Compare August 19, 2026 12:48
@frederickobrien frederickobrien added the run_chromatic Runs chromatic when label is applied label Aug 19, 2026
@github-actions github-actions Bot removed the run_chromatic Runs chromatic when label is applied label Aug 19, 2026
@frederickobrien
frederickobrien force-pushed the move-picture-layout-into-grid branch from 2b18bb3 to 7e10e7c Compare August 19, 2026 13:03
id="maincontent"
css={[
`margin-top: ${remSpace[3]}`,
`margin-top: ${format.design === ArticleDesign.Picture ? 0 : remSpace[3]}`,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This keeps parity with the Chromatic tests though I'd argue it's better for this to be consistent

@frederickobrien frederickobrien added the run_chromatic Runs chromatic when label is applied label Aug 19, 2026
@github-actions github-actions Bot removed the run_chromatic Runs chromatic when label is applied label Aug 19, 2026
@frederickobrien

Copy link
Copy Markdown
Contributor Author

There appear to be some unintentional differences now: This PR: Screenshot 2026-08-12 at 09 39 14

prod: Screenshot 2026-08-12 at 09 39 21

I think I've addressed these as part of some rebasing and polishing. There are a handful of Chromatic changes now but I think they're preferable if that makes sense?

@Jakeii

Jakeii commented Aug 25, 2026

Copy link
Copy Markdown
Member

There appear to be some unintentional differences now: This PR: Screenshot 2026-08-12 at 09 39 14
prod: Screenshot 2026-08-12 at 09 39 21

I think I've addressed these as part of some rebasing and polishing. There are a handful of Chromatic changes now but I think they're preferable if that makes sense?

Could you add a story for the case I found perhaps? Also have you done any other manual testing? As clearly the stories aren't exhaustive!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Departmental tracking: maintenance work, not a fix or a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants