Skip to content
This repository has been archived by the owner on Jan 17, 2024. It is now read-only.

Replaces custom breadcrumb component with govuk-component #509

Merged
merged 1 commit into from
Oct 5, 2020

Conversation

aliuk2012
Copy link
Contributor

@aliuk2012 aliuk2012 commented Sep 21, 2020

Context

Apologies for the size of this PR, as you will see from the file changes, this will standardise our approach to breadcrumbs. Currently, there is a mixture of hardcoded markup in the view or a view helper method which uses a custom view component under the hood.

 <% breadcrumbs([{ "Home" => school_home_path }, title ]) %>

part of #466

Changes proposed in this pull request

  • Replaces custom breadcrumb component with govuk-component version. All that's needed is to pass the component a hash with key name being the link copy and the value being a path for that breadcrumb link. if the value is null then it will just output the key name but not be considered a link.

Guidance to review

@muqi1 muqi1 temporarily deployed to ghwt-review-pr-509 September 21, 2020 14:16 Inactive
@aliuk2012 aliuk2012 temporarily deployed to ghwt-review-pr-509 September 21, 2020 15:24 Inactive
@aliuk2012 aliuk2012 marked this pull request as ready for review September 21, 2020 15:28
{ 'Home' => responsible_body_home_path },
title,
]) %>
<%= render GovukComponent::Breadcrumbs.new(breadcrumbs:{
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to get rid of the syntactic sugar? (breadcrumbs is much punchier than render GovukComponent::Breadcrumbs.new(breadcrumbs:), and the "#{title}": '' could also be cleaner?

Copy link
Contributor Author

@aliuk2012 aliuk2012 Sep 21, 2020

Choose a reason for hiding this comment

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

Initially, I thought it would be best to try reduce all the extra fluff and just call the component directly but on second thought seeing the actual number of times we use the breadcrumbs it does seem rather long winded for an easier to remember call to breadcrumbs. Also using the older rocket style for the hash will make it tidier to read instead of all the string interpolations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benilovj I've taking another stab at replacing the breadcrumbs. I've kept the view helper method as we discussed. I think this is good for another review and potential merge.

@aliuk2012 aliuk2012 changed the title Replaces custom breadcrumb component with govuk-component [WIP] Replaces custom breadcrumb component with govuk-component Sep 25, 2020
@aliuk2012 aliuk2012 temporarily deployed to ghwt-review-pr-509 September 29, 2020 09:26 Inactive
`govuk-components` gem includes a breadcrumb component. We should use this
instead of maintaining our own custom one. This commit also replaces all
the hardcoded breadcrumbs and standardises the breadcrumbs.

I've still kept the breadcrumb view helper method for syntactic sugar.
Inside this method I had to transform the array that comes in as it can
include a hash (key is breadcrumb link text and value is breadcrumb path)
and a string (last breadcrumb in the list and not usually linked to).
@aliuk2012 aliuk2012 temporarily deployed to ghwt-review-pr-509 October 2, 2020 21:56 Inactive
@aliuk2012 aliuk2012 changed the title [WIP] Replaces custom breadcrumb component with govuk-component Replaces custom breadcrumb component with govuk-component Oct 2, 2020
@aliuk2012 aliuk2012 merged commit 264cfed into main Oct 5, 2020
@aliuk2012 aliuk2012 deleted the replace-breadcrumbs branch October 5, 2020 08:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants