Flexible Page Basic Structure by KludgeKML · Pull Request #4718 · alphagov/frontend

KludgeKML · 2025-03-24T15:03:55Z

, Jira issue PNP-6387⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

What

Adds basic support for Flexible Pages, and two section types (page_title and rich_content), suitable for making a basic version of the existing history pages. Also adds an example history page loaded from local content at test/flexible-page

(Note you will need ALLOW_LOCAL_CONTENT_ITEM_OVERRIDE=true set to see this item - I have set it on the preview app).

Example on preview app:
https://govuk-frontend-app-pr-4718.herokuapp.com/test/flexible-page

Why

alphagov/govuk-rfcs#182

Trello card: https://trello.com/c/Bwy1WiJW

leenagupte

This is looking good. I've added a few inline comments, mainly where there's a missing test.

leenagupte · 2025-04-24T15:38:32Z

+    attr_reader :data, :flexible_page, :type
+
+    def initialize(flexible_section_hash, flexible_page)
+      @data = flexible_section_hash


Calling this data has tripped me up in a later commit as I forgot it was assigned to flexible_section_hash. Perhaps instead it should be renamed to flexible_section_hash as that is what it is. Having two names is confusing.

Hmm, this is tricky. The parameter name is "what the calling method thinks it's sending you", and the attribute is "what the object thinks of this", so I think it's legit to have them have two names (it's one name for each domain). That said, @data could be something like source_hash, maybe?

Yeah, I think source_hash works. Although to be fair, in the ContentItem model we did keep content_store_response: https://github.com/alphagov/frontend/blob/main/app/models/content_item.rb#L11

I'm going to change data to flexible_section_hash, as per our discussion.

andysellick

I won't approve for now as there's some comments still to address from other reviewers, but I'm happy with this code, good job 👏

leenagupte

Thank for adding the tests.

I have a few inline questions about some of the naming. Otherwise it looks good. 🎉

leenagupte · 2025-04-30T12:11:27Z

+    attr_reader :data, :flexible_page, :type
+
+    def initialize(flexible_section_hash, flexible_page)
+      @data = flexible_section_hash


Yeah, I think source_hash works. Although to be fair, in the ContentItem model we did keep content_store_response: https://github.com/alphagov/frontend/blob/main/app/models/content_item.rb#L11

leenagupte · 2025-04-30T12:15:53Z

+  include described_class
+
+  describe "#render_flexible_section" do
+    it "contains error items" do


Minor: I think you could rename this to it "renders the correct partial for the flexible section" rather than talking about errors.

Solving this problem by removing the whole helper.

leenagupte · 2025-04-30T12:19:19Z

+  class Base
+    attr_reader :data, :flexible_page, :type
+
+    def initialize(flexible_section_hash, flexible_page)


I was getting very confused about what flexible_page is here. But it's the content item, right? Perhaps then it should be called content_item?

I'm assuming the whole content item is being passed in here so that the links hash among others, can still be accessed?

leenagupte · 2025-04-30T12:21:14Z

@@ -0,0 +1,11 @@
+class FlexiblePage::FlexibleSectionFactory
+  def self.build(flexible_section_hash, flexible_page)


Also here, flexible_page the full content_item right?

Changing this to content_item.

leenagupte · 2025-04-30T12:23:15Z

 <% end %>

+<div class="govuk-width-container">
+  <% @content_item.flexible_sections.each do |flexible_section| %>


Nitpick: You should be able to use content_item here.

leenagupte · 2025-04-30T12:26:02Z

+    def initialize(flexible_section_hash, flexible_page)
+      super
+
+      @context = data["context"]


This is where the @data naming tripped me up. I'd forgotten that @data == flexible_section_hash so then I had to go looking to see where it came from. I think I had the same name association issues in the landing page models.

leenagupte · 2025-04-30T12:28:00Z

+<div class="govuk-width-container">
+  <% @content_item.flexible_sections.each do |flexible_section| %>
+    <div class="govuk-grid-row">
+      <%= render_flexible_section(flexible_section) %>


Not a blocker: Is this helper method only used here? If so, I'd ask whether it's actually needed.

Solving by removing helper.

leenagupte · 2025-04-30T12:42:10Z

+      @govspeak = data["govspeak"]
+
+      if data["image"].present?
+        alt, src = data.fetch("image").values_at("alt", "src")


I forgot to mention in the previous review, this is a nice use of fetch with values_at.

leenagupte · 2025-04-30T12:51:17Z


+<% content_for :before_content do %>
+  <div class="govuk-width-container">
+    <%= render "govuk_publishing_components/components/contextual_breadcrumbs", content_item: content_item.to_h %>


This is much nicer than having to hard-code the breadcrumb 🎉

- This fixture is over-detailed at this point, but matches the local content item for development that will be added later. It's necessary to have a file to run the request test (it doesn't have to be this detailed, but it doesn't hurt). It's modelled after the No 10 history page: https://www.gov.uk/government/history/10-downing-street

- At this point we have a controller, a route, and a view, so we can add the basic tests we're currently doing as review tests (basic 200, and correct template rendered)

- all flexible sections will have a type, the data they were initialised with, and a link to the content item (ie flexible page) that contains them. - factory works similar to the exist landing page block factory: https://github.com/alphagov/frontend/blob/7ae68c66b9308f1c45dab8cc2b56ee1814b9b11e/app/models/landing_page/block_factory.rb ...but with the simplification that we don't need to provide as much error handling, and we don't need a build_all method because there are not going to be any nested sections.

- We add a loop in the view to find renderable views for the sections and render them in place.

- This flexible section renders a page title with an optional context, using the heading component. Co-authored-by: Andy Sellick <andy.sellick@digital.cabinet-office.gov.uk>

- This flexible section renders a 1/3rd 2/3rd section of page, in which the left (1/3) side is a contents list for the right (2/3) section, which is govspeak. - Also add a contents list presenter that will return the content list in a format required by the contents list component. Co-authored-by: Andy Sellick <andy.sellick@digital.cabinet-office.gov.uk>

- This page modelled on the Number 10 history page https://www.gov.uk/government/history/10-downing-street

- System tests for all flexible page sections to be added here (it may be that we make those effectively individual view tests later, similar to components, but for the moment it's simplest to add here)

- Here we're relying on the contextual breadcrumb component to make a breadcrumb using the links/parent elements in the content item, but because of the full-width layout we have to opt out of the breadcrumb code in the layout and explicitly render it inside a govuk-width container.

leenagupte

Thanks for updating the naming 🥇

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 March 24, 2025 15:04 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 March 24, 2025 16:35 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 March 27, 2025 13:47 Inactive

andysellick mentioned this pull request Mar 27, 2025

[DO NOT MERGE] Flexible pages structure with content #4725

Closed

KludgeKML force-pushed the flexible-pages-structure branch from a7433a6 to 3ee0962 Compare March 27, 2025 15:10

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 March 27, 2025 15:11 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 March 27, 2025 16:48 Inactive

KludgeKML force-pushed the flexible-pages-structure branch from 8b7df2a to 76709a7 Compare March 27, 2025 16:54

KludgeKML changed the title ~~WIP~~ Flexible Page Basic Structure Mar 27, 2025

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 March 27, 2025 16:54 Inactive

KludgeKML force-pushed the flexible-pages-structure branch from 76709a7 to aef2b63 Compare March 27, 2025 17:01

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 March 27, 2025 17:02 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 April 2, 2025 15:57 Inactive

KludgeKML force-pushed the flexible-pages-structure branch from 35b697c to 61645c0 Compare April 3, 2025 08:29

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 April 3, 2025 08:30 Inactive

KludgeKML force-pushed the flexible-pages-structure branch from 61645c0 to f9e2cdf Compare April 3, 2025 08:55

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 April 3, 2025 08:56 Inactive

KludgeKML force-pushed the flexible-pages-structure branch from f9e2cdf to 8c9a631 Compare April 3, 2025 08:59

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 April 3, 2025 09:00 Inactive

KludgeKML force-pushed the flexible-pages-structure branch from 8c9a631 to ba7d783 Compare April 3, 2025 09:18

KludgeKML marked this pull request as ready for review April 3, 2025 09:18

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 April 3, 2025 09:18 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 April 3, 2025 09:33 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 April 3, 2025 09:39 Inactive

KludgeKML force-pushed the flexible-pages-structure branch from a06069d to 34eed93 Compare April 3, 2025 10:27

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 April 3, 2025 10:27 Inactive

KludgeKML force-pushed the flexible-pages-structure branch from 34eed93 to 32661d8 Compare April 3, 2025 10:36

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 April 3, 2025 10:36 Inactive

KludgeKML force-pushed the flexible-pages-structure branch from 32661d8 to 73ae6db Compare April 3, 2025 10:44

KludgeKML requested a review from andysellick April 3, 2025 10:44

KludgeKML requested a review from deborahchua April 24, 2025 13:43

leenagupte reviewed Apr 24, 2025

View reviewed changes

andysellick reviewed Apr 25, 2025

View reviewed changes

KludgeKML force-pushed the flexible-pages-structure branch from 0cd00af to 00bd3b2 Compare April 28, 2025 11:33

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 April 28, 2025 11:33 Inactive

KludgeKML force-pushed the flexible-pages-structure branch from 00bd3b2 to 3faaca5 Compare April 28, 2025 13:40

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 April 28, 2025 13:41 Inactive

KludgeKML force-pushed the flexible-pages-structure branch from 3faaca5 to 16d3be6 Compare April 28, 2025 16:02

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 April 28, 2025 16:02 Inactive

KludgeKML force-pushed the flexible-pages-structure branch from 16d3be6 to ad426c2 Compare April 29, 2025 08:58

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 April 29, 2025 08:58 Inactive

KludgeKML requested a review from leenagupte April 29, 2025 08:58

leenagupte reviewed Apr 30, 2025

View reviewed changes

KludgeKML and others added 11 commits April 30, 2025 14:54

Add basic route/controller/view for Flexible Page

b2f9a26

Add request spec

c3d7099

- At this point we have a controller, a route, and a view, so we can add the basic tests we're currently doing as review tests (basic 200, and correct template rendered)

Add empty flexible_page model

2f0c0af

Add FlexibleSection attribute to the FlexiblePage model and view

8b5ab86

- We add a loop in the view to find renderable views for the sections and render them in place.

Add PageTitle flexible section

ba4ad71

- This flexible section renders a page title with an optional context, using the heading component. Co-authored-by: Andy Sellick <andy.sellick@digital.cabinet-office.gov.uk>

Add example history page

fd1d697

- This page modelled on the Number 10 history page https://www.gov.uk/government/history/10-downing-street

Add system test

44838bd

- System tests for all flexible page sections to be added here (it may be that we make those effectively individual view tests later, similar to components, but for the moment it's simplest to add here)

KludgeKML force-pushed the flexible-pages-structure branch from ad426c2 to 48f8c96 Compare April 30, 2025 13:56

govuk-ci temporarily deployed to govuk-frontend-app-pr-4718 April 30, 2025 13:57 Inactive

KludgeKML requested a review from leenagupte April 30, 2025 13:58

leenagupte approved these changes Apr 30, 2025

View reviewed changes

KludgeKML merged commit 0f0fd9c into main Apr 30, 2025

KludgeKML deleted the flexible-pages-structure branch April 30, 2025 15:34

		@@ -0,0 +1,11 @@
		class FlexiblePage::FlexibleSectionFactory
		def self.build(flexible_section_hash, flexible_page)

Conversation

KludgeKML commented Mar 24, 2025 • edited by deborahchua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Uh oh!

leenagupte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andysellick left a comment

Choose a reason for hiding this comment

Uh oh!

leenagupte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leenagupte left a comment

Choose a reason for hiding this comment

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

KludgeKML commented Mar 24, 2025 •

edited by deborahchua

Loading