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

Update publication pages #2302

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Update publication pages #2302

wants to merge 2 commits into from

Conversation

chris-gds
Copy link

@chris-gds chris-gds commented Nov 25, 2021

What

Apply the context_inside option to the title component to all publications, remove the word "overview" yet keep translations.

Why

Recently some work made the 'grey text' of publication pages h1. This provided various benefits including for screen readers and tabs. However, we only did this for pages where the title of the page and attachment are the same. For consistency this change applies to all publication pages. So that we don't have some pages that operate in one way, and others that do it differently. Also it adds benefits.

Visuals

image

Anything else

Checked against:
http://government-frontend.dev.gov.uk/government/consultations/appraisal-periods-consultation

Add context title
Consultation outcome: Appraisal periods consultation

http://government-frontend.dev.gov.uk/government/publications/information-superiority-jdn-213

Adds withdrawn and keep context title
[Withdrawn] Guidance: Information superiority: more than managing information

http://government-frontend.dev.gov.uk/government/organisations/department-of-health-and-social-care/about

No context title
About us - Department of Health and Social Care

http://government-frontend.dev.gov.uk/government/statistical-data-sets/cw010-proportion-of-residents-walking-or-cycling-at-least-once-a-month

Adds withdrawn and no context title (as not a publication or consultation)
[Withdrawn] Proportion of residents who do any walking or cycling (at local authority level) (CW010)

Related PRs

#2238
#2292

@govuk-ci govuk-ci temporarily deployed to government-f-update-pub-a42nba November 25, 2021 14:46 Inactive
@govuk-ci govuk-ci temporarily deployed to government-f-update-pub-a42nba November 25, 2021 17:15 Inactive
@chris-gds chris-gds marked this pull request as ready for review November 25, 2021 17:53
@chris-gds chris-gds marked this pull request as draft November 25, 2021 18:04
@govuk-ci govuk-ci temporarily deployed to government-f-update-pub-a42nba November 25, 2021 18:15 Inactive
@govuk-ci govuk-ci temporarily deployed to government-f-update-pub-a42nba November 26, 2021 17:28 Inactive
Copy link
Contributor

@maxgds maxgds left a comment

Choose a reason for hiding this comment

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

Couple of things look to need addressing, one simple and one more challenging.

app/presenters/content_item/withdrawable.rb Outdated Show resolved Hide resolved
app/views/content_items/_context_and_title.html.erb Outdated Show resolved Hide resolved
@govuk-ci govuk-ci temporarily deployed to government-f-update-pub-a42nba November 30, 2021 15:38 Inactive
@govuk-ci govuk-ci temporarily deployed to government-f-update-pub-a42nba November 30, 2021 15:50 Inactive
@govuk-ci govuk-ci temporarily deployed to government-f-update-pub-a42nba December 1, 2021 09:45 Inactive
@chris-gds chris-gds marked this pull request as ready for review December 1, 2021 09:52
@chris-gds chris-gds requested a review from maxgds December 1, 2021 09:52
@chris-gds chris-gds marked this pull request as draft December 1, 2021 12:00
@govuk-ci govuk-ci temporarily deployed to government-f-update-pub-a42nba December 1, 2021 15:27 Inactive
@govuk-ci govuk-ci temporarily deployed to government-f-update-pub-a42nba December 1, 2021 15:35 Inactive
Chris Yoong added 2 commits December 1, 2021 15:47
The context_label is being defined by "schema_name: "publication" - should this be present this determines whether we class this as a "publication or consultation"

Ensure title of page has "context_inside" Content prefixed to a "publication or consultation"

Should content be [withdrawn] this also adds the "context_label" (or grey text) to the title scoped to items that have been identified as a "publication or consultation"

Remove the loops around the attachments and just apply the context_label in all cases.

Remove word "overview" from "context_inside"
No longer required
@govuk-ci govuk-ci temporarily deployed to government-f-update-pub-a42nba December 1, 2021 15:48 Inactive
@chris-gds chris-gds marked this pull request as ready for review December 1, 2021 15:52
end

def publication_overview
overview = (I18n.exists?("content_item.#{schema_name}") == "publication" || "statistical_data_set")
Copy link
Contributor

@maxgds maxgds Dec 1, 2021

Choose a reason for hiding this comment

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

Could you expand a little on what's going on here to help me understand? I have two questions:

  1. Why is statistical_data_set relevant? It's not a sub type of publication so we never expect to apply this overview/context title behaviour to it.
  2. Why are you looking up whether there is a translation for the schema_name ? Don't we want to always apply overview if it's a publication?

It feels like this could be as simple as line 32 - 34 being

    def context_title?
      content_item.#{schema_name} == "publication"
    end

and lines 36-44 being removed, but I expect I'm missing something. Or multiple somethings.

@chris-gds chris-gds marked this pull request as draft December 3, 2021 07:50
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.

None yet

3 participants