Render featured attachments directly #1709
Conversation
Previously the main content of a publication was extracted into a single-use partial, which made the view harder to visualise in code. This inlines the partial into the main view.
Previously publication and consultation attachments were rendered inline. This extracts the rendering into a new 'attachments' partial, which has the nice side effect of improving the aria markup for consultations, as well as making the testing approach consistent. This also replaces the 'documents' question methods in each presenter with explicit calls in the views. Since the view needs to be aware of the type of data, it makes sense to ask questions like 'present?' and 'any?' in the view directly, instead of bloating the presenters.
benthorner
force-pushed
the
featured-attachments
branch
from
7e27ae2 to
999f56c
Compare
This extends the attachments partial to render attachments directly using the new 'attachments' metadata in the payload (see RFC 116) [1]. Since both kinds of data may be present in the payload, I've added a view test to ensure we don't change the way attachments are rendered until the publishing app stops sending pre-rendered attachments. Publications have one set of featured attachments, while consultations can have up to three. For the tests, I decided to test both the legacy and the new behaviours in the same test block, noting that there's no other way to group these related tests together. The new 'attachment' CSS class replicates the 30px bottom margin that legacy pre-rendered attachments have in Govspeak [2]. [1] https://github.com/alphagov/govuk-rfcs/blob/master/rfc-116-store-attachment-data-in-content-items.md [2] https://github.com/alphagov/govuk_publishing_components/blob/6ee40d5f1c2b3d81d98db0923ab96cf454d34ff0/app/assets/stylesheets/govuk_publishing_components/components/govspeak/_attachment.scss#L22
benthorner
force-pushed
the
featured-attachments
branch
from
999f56c to
a097b81
Compare
| @@ -29,7 +29,27 @@ | |||
| <div class="govuk-grid-column-two-thirds responsive-bottom-margin"> | |||
| <%= render 'components/important-metadata', items: @content_item.important_metadata %> | |||
| <div class="responsive-bottom-margin"> | |||
| <%= render 'publication_inline_body' %> | |||
| <%= render 'govuk_publishing_components/components/heading', | |||
| text: t("publication.documents", count: 5), # This should always be pluralised. | |||
There was a problem hiding this comment.
Maybe in another commit after the first commit, you could change this. The count: 5 feels weirdly arbitrary. Even 2 would be clearer!
There was a problem hiding this comment.
It's tempting, but that means going through all 61 locale files and working out what to do with it. There's definitely a bigger problem with the use of I18n in this app, so I'd like to leave it for someone else to tackle.
| text: title, | ||
| mobile_top_margin: true %> | ||
|
|
||
| <div aria-labelledby="<%= attachment_title_id %>"> |
There was a problem hiding this comment.
Wouldn't this have the effect of reading the heading out twice (once on the heading itself, and once immediately afterwards)? (NB, the examples I've seen on MDN tend to show the heading inside the div):
<div aria-labelledby="<%= attachment_title_id %>">
<%= render 'govuk_publishing_components/components/heading',
id: attachment_title_id,
text: title,
mobile_top_margin: true %>I'm sure Alex has already looked at this, I'm just not immediately clear on how this is an improvement 🤔
There was a problem hiding this comment.
Looks like this is a case of me assuming we were doing a good thing in the first place 🤦♂. This is replicating a behaviour that previously applied only to publication documents, and applying it to all lists of attachments (with a title).
Apparently it was introduced here, but with no explanation. I've added another commit so it conforms to the spec. Removing it makes the tests harder, as there's nothing to 'grab hold of', so I've gone with fixing it, instead.
How does it look now?
| within '[aria-labelledby="download-the-full-outcome-title"]' do | ||
| assert page.has_text?("Employee Share Schemes: NIC elections - consulation response") | ||
| end | ||
|
|
||
| setup_and_visit_content_item("consultation_outcome_with_featured_attachments") |
There was a problem hiding this comment.
Each of these test blocks seem like they should be split into two test blocks - one for as-is, and one for directly. I appreciate there's no other way of grouping them together, save for a comment in between every two blocks, but it would make it easier to delete the as-is blocks when we come to deprecate that behaviour.
There was a problem hiding this comment.
While I agree they could be split up, I think that would decrease the readability of the rest of the test file, as there is indeed no way of grouping them, and they are effectively testing the same thing. One of the commit messages says this, somewhere.
There was a problem hiding this comment.
I don't think I would find it that much harder to delete the lines within the test, when we (finally) disable the legacy functionality.
There was a problem hiding this comment.
Thanks Ben, the ordering looks better now. I think they say best practice is to remove role="region" and swap out the <div> for a <section>; the semantics of the element automatically conveys that it is a region: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/Region_role
benthorner
force-pushed
the
featured-attachments
branch
from
f9d46f6 to
cda15fc
Compare
|
Ah, that's useful to know, thanks. I've updated it to use |
There was a problem hiding this comment.
Whoops, I forgot to say - we can also remove the aria-labelledby, as the section will take this from the first heading. This is loosely implied in https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/Region_role, but more explicit in https://developer.mozilla.org/en-US/docs/Web/HTML/Element/section#Usage_notes.
Not a blocker, so I'll leave it up to you.
benthorner
force-pushed
the
featured-attachments
branch
from
cda15fc to
5b3c229
Compare
benthorner
force-pushed
the
featured-attachments
branch
from
5b3c229 to
26d94af
Compare
|
Ah, thanks for pointing that out - I like learning a new thing, even if it's dubiously documented 👓. I've updated the last commit to implement that, which has the nice effect of simplifying the tests 👍. |
Previously the div of attachments used 'aria-labelledby' to refer to an outer heading element, which is incorrect: the label should refer to something inside the 'div'. The 'div' should also set the appropriate 'role="section"' attribute [1], or we can just replace the 'div' with a 'section' element [2]. This corrects the structure of the attachments partial so that the sections properly self-describe. Each section now has an ID, so they continue to be easy to grab hold of in tests. [1] https://www.w3.org/TR/wai-aria/#region [2] https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/Region_role
benthorner
force-pushed
the
featured-attachments
branch
from
26d94af to
bf24c71
Compare
https://trello.com/c/wM9wqe9H/1587-support-featured-attachments-directly-on-govuk-frontend
Depends on: alphagov/govuk-content-schemas#975
This changes government-frontend to render attachments
directly, when a pre-rendered payload is not supplied (see RFC 116) [1].
Note that we no longer support PDF preview for documents rendered
using this new method. This won't impact any documents at the moment,
since all of them still have a pre-rendered 'documents' payload.
Before (Publication)
After (Publication)
Before (Consultation)
After (Consultation)
Before (Mobile)
After (Mobile)
[1]
https://github.com/alphagov/govuk-rfcs/blob/master/rfc-116-store-attachment-data-in-content-items.md