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

Swap out govuk_link_to with govuk-component version #513

Merged
merged 1 commit into from
Sep 23, 2020

Conversation

aliuk2012
Copy link
Contributor

@aliuk2012 aliuk2012 commented Sep 21, 2020

Context

Changes proposed in this pull request

  • replaces custom govuk_link_to with govuk-component govuk_link_to helper method (source)

There is only one major difference between govuk-component helper method implementation and the one in the service. The difference is that govuk-component helper method does not support passing in a block. I don't see this as a blocker as I could only see one instance where a block was used and I assume the reason it was a block was because it was using html markup and rails would have been escaping it. This could have been overcome by using html_safe.

Guidance to review

There seemed to be only one govuk_link_to which was using a block.
I assume the reason it was a block was because it was using html markup
and rails would have been escaping it. This can be overcome by using
html_safe.
@muqi1 muqi1 temporarily deployed to ghwt-review-pr-513 September 21, 2020 14:26 Inactive
@aliuk2012 aliuk2012 marked this pull request as ready for review September 21, 2020 14:34
<%= govuk_link_to('', new_responsible_body_internet_mobile_manual_request_path( anchor: 'extra-mobile-data-request-account-holder-name-field') ) do %>
Change<span class="govuk-visually-hidden"> Account holder name</span>
<%- end %>
<%= govuk_link_to('Change<span class="govuk-visually-hidden"> Account holder name</span>'.html_safe,
Copy link
Contributor

Choose a reason for hiding this comment

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

(We should push the block logic upstream to govuk-components, so that govuk_link_to continues to act like the native link_to does.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll take that up with govuk-components but I don't see that being a blocker for us to get this merged.

@fofr
Copy link
Contributor

fofr commented Sep 22, 2020

Copy link
Contributor

@fofr fofr left a comment

Choose a reason for hiding this comment

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

Looks good.

@aliuk2012 aliuk2012 merged commit 61dd5c1 into main Sep 23, 2020
@aliuk2012 aliuk2012 deleted the replace-govuk-link-to branch September 23, 2020 09:53
@aliuk2012 aliuk2012 mentioned this pull request Oct 4, 2020
5 tasks
@benilovj
Copy link
Contributor

benilovj commented Oct 8, 2020

I've raised the issue of govuk_link_to not taking blocks here: x-govuk/govuk-components#65

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.

None yet

4 participants