-
Notifications
You must be signed in to change notification settings - Fork 20
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 breadcrumbs based on organisation, if no parent linked #4270
Use breadcrumbs based on organisation, if no parent linked #4270
Conversation
6aa30f8
to
3cd9e50
Compare
3cd9e50
to
16bbf0e
Compare
16bbf0e
to
3abaf50
Compare
3abaf50
to
69a27bc
Compare
69a27bc
to
4dd674a
Compare
4dd674a
to
7ecba90
Compare
7ecba90
to
bfc68be
Compare
bfc68be
to
f3a86f0
Compare
f3a86f0
to
cc89f22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had a quick look and have one thought on the updated breadcrumbs, using the page below as an example:
www.gov.uk/government/organisations/government-actuarys-department/about
The breadcrumbs are updated from Home > Government
to Home > Government Actuary's Department
which definitely feels like a nice a improvement.
It does appear that this page also has a grandparent, "Departments, agencies and public bodies", I think we should check with a content designer if this should also be included in the breadcrumbs as well, for example:
Home > Departments, agencies and public bodies > Government Actuary's Department
Unfortunately, I can't find the information about the higher levels in the content item, so getting this information would require either adding things to the content item (probably with the publishing team's help) or doing a separate call to the api to retrieve that, which seems like an overkill. If we want/can modify the content item in the first place, we can add a parent link instead. This would resolve the issue, as the logic prioritises parent links. Also some examples of corporate information pages in publishing-api have parent links, so there shouldn't be a problem with adding it. So maybe we can ask the publishing team to add the parent link and in the meantime we can use this solution? |
Or maybe we could use something like |
cc89f22
to
2768e72
Compare
2768e72
to
cd7167a
Compare
This change has now been applied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me from a frontend perspective
@hannako please can you help review the backend changes
cd7167a
to
bc57a30
Compare
bc57a30
to
0128619
Compare
0128619
to
0c31317
Compare
0c31317
to
ea3d65d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding all these tests Matt 🏅
I've requested a few tiny refactors. But my main question is on what valid corporate information pages look like
@@ -72,6 +76,10 @@ def content_has_curated_related_items? | |||
content_item.dig("links", "ordered_related_items").present? && content_item.dig("links", "parent").present? | |||
end | |||
|
|||
def content_has_related_organisations? | |||
ContentItem.new(content_item).related_organisations.present? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor:
I think we should follow the existing pattern in this file of just reading from the links. This method is just a boolean ie we don't need to do anything with the data. So unless I'm missing something, instantiating a new object feels unnecessary.
content_item.dig("links", "organisations").present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must match line 26 of lib/govuk_publishing_components/presenters/content_breadcrumbs_based_on_organisations.rb
So if this is to be changed, the other line needs to be changed as well.
It wouldn't be good, if we duplicated the code, as this wouldn't ensure the code (the list of methods) is always matching. Duplicating would easily lead to errors when it's changed in one place but not the other.
This solution calls the same, already existing method #related_organisations
in ContentItem
, which is a wrapper for raw content item data.
(Also this method contains additional logic which ensures the organisation we are interested in is always of the document type "organisation".
I know currently all organisations in links -> organisations are of "organisation" document type, so the additional logic is redundant right now.)
If we still wanted to change that, we'd need to create a method somewhere which would be called by both `lib/govuk_publishing_components/presenters/contextual_navigation.rb and lib/govuk_publishing_components/presenters/content_breadcrumbs_based_on_organisations.rb which would ensure that exactly the same list of organisations is used in both cases.
lib/govuk_publishing_components/presenters/breadcrumb_selector.rb
Outdated
Show resolved
Hide resolved
spec/lib/govuk_publishing_components/presenters/breadcrumb_selector_spec.rb
Outdated
Show resolved
Hide resolved
spec/lib/govuk_publishing_components/presenters/breadcrumb_selector_spec.rb
Show resolved
Hide resolved
...ib/govuk_publishing_components/presenters/content_breadcrumbs_based_on_organisations_spec.rb
Outdated
Show resolved
Hide resolved
ea3d65d
to
6ec19a8
Compare
6ec19a8
to
26b24a1
Compare
26b24a1
to
6ea9d7d
Compare
6ea9d7d
to
e6f74a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the notes, and for adding all the tests ⭐
Use breadcrumbs based on ancestors, if the document has a linked parent. Otherwise use breadcrumbs based on the first normal (non-world) organisation the page is linked to. If there are no linked organisations use taxon based breadcrumbs, or if taxons are also not linked to, the default breadcrumbs pointing to the homepage.
e6f74a7
to
20cb813
Compare
What
The Content Types Discovery team's audit uncovered that corporate_information pages lack a breadcrumb that connects back to the organisation.
Currently corporate information pages without the linked parent use taxon based breadcrumbs. An example would be the following page: https://www.gov.uk/government/organisations/government-actuarys-department/about
This PR fixes that, and if there is no parent linked, it uses the first linked organisation to generate breadcrumbs.
The logic is the following:
Why
So that users can navigate back to the organisation to which this corporate information relates.
Trello ticket
Testing
https://government-frontend-pr-3347.herokuapp.com/government/organisations/government-actuarys-department/about
https://government-frontend-pr-3347.herokuapp.com/government/organisations/department-for-business-energy-and-industrial-strategy/about/personal-information-charter
https://government-frontend-pr-3347.herokuapp.com/government/organisations/standards-and-testing-agency/about
https://government-frontend-pr-3347.herokuapp.com/government/organisations/hm-courts-and-tribunals-service/about
Visual Changes
Before
After