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

Use contextual navigation components #837

Merged
merged 1 commit into from
Mar 26, 2018
Merged

Conversation

tijmenb
Copy link
Contributor

@tijmenb tijmenb commented Mar 19, 2018

This PR uses the contextual navigation sidebar and breadcrumbs from the publishing components gem (alphagov/govuk_publishing_components#228). It removes all the logic for what to display where and replaces it with calls to the components.

The intention is that this PR doesn't change the layout, but there are a few changes:

  • Contacts have the new sidebar, but the data has stayed the same. I'm unsure why universal layout hadn't been implemented for contacts in the first place, and Ganesh wasn't sure either. This might have been an oversight.
  • The contacts breadcrumb has changed a little bit from "Contact HM Revenue & Customs" to "HM Revenue & Customs Contacts". This is because all contacts have the parent link to the contacts finder. I think this is a fine change. If we want to change the title we should change the finder title. The finder has been renamed so there are no more changes now (Rename the finder to "Contact HM Revenue & Customs" contacts-admin#372).
  • On topical event pages about pages, there's no more "(archived)" in the title when the topical event is archived. I think this is an acceptable trade-off given the complexity it would mean moving this special functionality to the components.
  • We no longer override the header navigation type for things tagged tagged to the taxonomy. I don't think there was very much reason behind showing it and not showing it, so I'm okay with this change (this is the content_for :simple_header stuff that's removed)
  • Even though there's a related_items method in the travel advice presenter, it was unused and the travel advice pages are unchanged after this PR.

Can be reviewed now, but don't merge until it's using a released version of alphagov/govuk_publishing_components#228.

https://trello.com/c/1LEofaHC

@tijmenb tijmenb had a problem deploying to government-frontend-pr-837 March 19, 2018 08:37 Failure
@tijmenb tijmenb force-pushed the contextual-navigation branch from 3200510 to 5086b5c Compare March 19, 2018 08:40
@tijmenb tijmenb temporarily deployed to government-frontend-pr-837 March 19, 2018 08:40 Inactive
Copy link
Contributor

@vanitabarrett vanitabarrett left a comment

Choose a reason for hiding this comment

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

On contact pages, the navigation sidebar isn't quite in the right place - it should be in line with the start of the paragraph text (just below the title), not above the title.

https://government-frontend-pr-837.herokuapp.com/government/organisations/hm-revenue-customs/contact/income-tax-enquiries-for-individuals-pensioners-and-employees (contact page)

vs

https://www.gov.uk/dvla-change-address (answer page)

@tijmenb
Copy link
Contributor Author

tijmenb commented Mar 19, 2018

@vanitabarrett thank you! updated.

@tijmenb tijmenb temporarily deployed to government-frontend-pr-837 March 19, 2018 09:51 Inactive
@vanitabarrett vanitabarrett dismissed their stale review March 19, 2018 09:52

This has been fixed

@tijmenb tijmenb force-pushed the contextual-navigation branch from faa122b to 533dda5 Compare March 19, 2018 17:13
@tijmenb tijmenb temporarily deployed to government-frontend-pr-837 March 19, 2018 17:13 Inactive
@tijmenb tijmenb force-pushed the contextual-navigation branch from 533dda5 to 8672b4a Compare March 20, 2018 14:20
@tijmenb tijmenb temporarily deployed to government-frontend-pr-837 March 20, 2018 14:20 Inactive
@tijmenb tijmenb force-pushed the contextual-navigation branch from 8672b4a to efda726 Compare March 20, 2018 14:39
@tijmenb tijmenb temporarily deployed to government-frontend-pr-837 March 20, 2018 14:39 Inactive
@tijmenb tijmenb force-pushed the contextual-navigation branch from efda726 to 0535030 Compare March 21, 2018 12:17
@tijmenb tijmenb temporarily deployed to government-frontend-pr-837 March 21, 2018 12:17 Inactive
@tijmenb tijmenb force-pushed the contextual-navigation branch from 0535030 to 1eac408 Compare March 21, 2018 13:56
@tijmenb tijmenb temporarily deployed to government-frontend-pr-837 March 21, 2018 13:56 Inactive
This PR uses the contextual navigation sidebar and breadcrumbs from the
publishing components gem. It removes all the logic for what to display
where and replaces it with calls to the components.

The intention is that this PR doesn't change the layout, but there are
a few changes:

- Contacts have the new sidebar, but the data has stayed the same. I'm
unsure why universal layout hadn't been implemented for contacts in the
first place, and Ganesh wasn't sure either. This might have been an
oversight.
- On topical event pages about pages, there's no more "(archived)" in
the title when the topical event is archived. I think this is an
acceptable trade-off given the complexity it would mean moving this
special functionality to the components.
- We no longer override the header navigation type for things tagged
tagged to the taxonomy. I don't think there was very much reason behind
showing it and not showing it, so I'm okay with this change (this is
the content_for :simple_header stuff that's removed)
- Even though there's a related_items method in the travel advice
presenter, it was unused and the travel advice pages are unchanged
after this PR.

https://trello.com/c/1LEofaHC
@tijmenb tijmenb merged commit 3d82ddf into master Mar 26, 2018
@tijmenb tijmenb deleted the contextual-navigation branch March 26, 2018 13:04
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