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

[WIP] [DO NOT MERGE] Add tracking to details elements in attachments on form pages #3048

Closed
wants to merge 1 commit into from

Conversation

andysellick
Copy link
Contributor

@andysellick andysellick commented Jan 15, 2024

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

What

Rework form pages to include tracking on details components that appear inside attachment components.

Specifically, rework the list of documents on the forms pages to use the content item data in details->attachments to render the required content, in place of the data in details->documents, which is pre-rendered.

This presents some problems, because the attachments partial used here is used elsewhere (consultations, call for evidence) where switching from a pre-rendered chunk of markup is not so straightforward. I've gone for a middle ground where attachments now accepts two parameters for the main content - either the pre-rendered markup (to keep the existing pages from breaking) or an array of data to be used in the template with components to generate the page - and updated all the places where the partial is called accordingly.

This change will impact form pages as well as some other page types. I'm a bit confused by the naming conventions, form pages are handled by publication code, but this will also impact call for evidence and consultation pages.

Example form pages:

Still to do:

  • attachment thumbnails are not included in the content item outside of the pre-rendered markup, will need to find a way to pass them
  • ...then republish so they come through in the content item
  • fix failing tests
  • apply tracking to the details components, where they appear

Why

Good question! We need to put tracking on the details components that can (sometimes) appear beneath the list of attachments, and that's very hard to do here on pre-rendered markup - normally we'd just pass some options to the component.

Screenshot 2024-01-22 at 13 59 49

Visual changes

None, hopefully.

Trello card: https://trello.com/c/xhASj1rp/765-add-tracking-to-details-component

@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3048 January 15, 2024 14:15 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3048 January 15, 2024 14:46 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3048 January 15, 2024 14:56 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3048 January 15, 2024 16:52 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3048 January 16, 2024 09:57 Inactive
- rework the list of documents on the forms pages to use the content item data in `details->attachments` to render the required content, in place of the data in `details->documents`, which is pre-rendered
- will make putting tracking onto these elements easier
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3048 January 16, 2024 11:02 Inactive
@andysellick
Copy link
Contributor Author

@leenagupte @hannako I've updated the PR to hopefully make it a bit clearer, but please shout if you have any questions.

Copy link
Contributor

@hannako hannako left a comment

Choose a reason for hiding this comment

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

First main question, out of scope of your PR but just for my understanding...why have we got duplicate attachment data in the content items?!

Second question - Is it more or less confusing to an unfamiliar (eg 2nd line) dev, to have the attachments partial render both types of attachment data?

I'm wondering if it might be simpler to have two partials, each handling one type. I imagine the long term plan would be to phase out the govspeak version (attachment_html)?

content_item["details"]["documents"].to_a.join("")
docs = content_item["details"]["attachments"].select { |a| a["locale"] == locale }
docs.each do |doc|
doc.delete("alternative_format_contact_email") if doc["accessible"] == true
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this about? Maybe this change belongs in its own commit with an explanation in the commit message.

Minor style comment: I find it a bit confusing that the documents method returns a subset of["details"]["attachments"]. I'd expect this code to follow the pattern set on line 21. Is there a reason we need to call this method documents? If not could we have an attachments method, and a filtered_attachments method (that contains the filtering logic), and is called in the templates? Maybe that's not possible if partials are being shared...

@andysellick
Copy link
Contributor Author

@hannako thanks for the review. Great idea on using an entirely new partial. I've raised a new PR here to take advantage of this approach and break the change into smaller commits: #3058 (currently less complete than this PR).

Excellent question - yes why do we have duplicate data in the content item? I have no idea, but I think generally we should have data like this rather than pre-rendered HTML, so I'm keen to use it. It'll need some adjusting to make it fully useful (it doesn't contain the URLs for the thumbnails for each attachment) - any idea where it's coming from?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants