New link helpers #419

peteryates · 2023-05-29T11:04:28Z

This PR introduces a new set of link helpers that will ~~be available as an option but will eventually become default~~ be the default. The legacy 100% Rails-compatible helpers will still be available for the time being by including the helper module as described below.

The current helpers are closely tied to Rails' implementation which after 18 years is showing its age. Because it pre-dates keyword arguments and can take many formats of link, the code is complex and difficult to extend.

The new implementation:

is clean, all of the logic is handled by Rails' class_names helper
does minimal argument juggling
supports opening links in new tabs
does not support the old-style verbose non-resource-orientated govuk_link_to("Show profile", controller: "profiles", action: "show", id: @profile) format. I haven't seen much evidence of this style in use and Rails' documentation advises against using it

Multiple versions of link helpers

For a while there will be two versions of the link helpers:

the new ones GovukLinkHelper
the old ones GovukRailsCompatibileLinkHelper

I originally planned to make choosing between them a config option but due to the order the gem is required and then the intializers run, it's a pain. Instead, the new ones will automatically be included and if the old ones are wanted they can be manually included in ApplicationHelper:

module ApplicationHelper
  include GovukRailsCompatibileLinkHelper
end

This will make the old method variants override the new ones.

I will begin adding some warnings to the old ones before they are dropped.

Final things to do

add some tests ensuring additional arguments (both options and html_options) can still be passed through to the Rails helpers
visually_hidden_prefix and visually_hidden_suffix keyword arguments?

netlify · 2023-05-29T11:04:34Z

✅ Deploy Preview for govuk-components ready!

Name	Link
🔨 Latest commit	`640125c`
🔍 Latest deploy log	https://app.netlify.com/sites/govuk-components/deploys/648731e8327eef0009320a77
😎 Deploy Preview	https://deploy-preview-419--govuk-components.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

app/helpers/govuk_new_link_helper.rb

frankieroberto · 2023-06-12T14:20:21Z

@peteryates I tried to look at this diff, but there’s a lot of changes so it's quite hard to follow (at least for me).

Would you mind summarising the main changes to the helpers that this PR proposes? Maybe a before & after would help?

peteryates · 2023-06-12T14:50:18Z

@frankieroberto yeah, good call. I added a replacement helper that has the same methods in it and I think the fact I changed the order has made the diff more difficult to digest than it should be. I'll see if I can improve it and write a full summary.

Essentially though the old helpers follow a format like this:

  def govuk_link_to(name = nil, options = nil, extra_options = {}, &block)
    extra_options = options if block_given?
    html_options = build_html_options(extra_options)

    if block_given?
      link_to(name, html_options, &block)
    else
      link_to(name, options, html_options)
    end
  end

As you can see here, options and extra_options are both hashes that are 'juggled' depending on whether link text or a block is provided. This makes the code hard to follow, especially once you look in build_html_options which goes to great lengths to merge the provided options with the ones required by the design system.

It's very brittle and caught me out when I tried to add the new_tab functionality as a keyword arg #363 some time ago. @HettieS and I tried the preview release with Register and it broke half the test suite because different versions of Ruby treat keyword args and argument hashes differently - so I retracted it entirely.

This change switches everything over to keyword arguments, allowing the code to be much simpler. Most projects shouldn't notice any change. It might bite some that are using the old style Rails format, but the Rails docs have advised against using them for years and I barely see any in the wild so I don't image it's widespread. They will be able to continue using the old ones for the time being.

peteryates · 2023-06-12T14:57:05Z

app/helpers/govuk_link_helper.rb

-      fail(ArgumentError, "invalid styles #{invalid_styles.to_sentence}. Valid styles are #{LINK_STYLES.keys.to_sentence}")
+  def govuk_link_classes(inverse: false, muted: false, no_underline: false, no_visited_state: false, text_colour: false)
+    if [text_colour, inverse, muted].count(true) > 1
+      fail("links can be either text_colour, inverse or muted - not combinations of the three")


This could definitely be worded better. Is it even needed at all? 🤔

frankieroberto · 2023-06-12T17:13:10Z

@peteryates ah ok, so it’d still be the case that the first argument would be the link text and the second one the path/url, but after that you’d use keyword arguments for any extra attributes or options?

peteryates · 2023-06-13T09:03:12Z

Yeah - exactly that. Then any GOV.UK specific ones will be picked up and used to assign the relevant classes/attributes and any extra ones (kwargs) will get passed through to the underlying Rails helper.

Essentially it's switching away from having to extract the GOV.UK specific stuff from a generic hash of arguments.

peteryates · 2023-07-17T07:35:58Z

This is likely to be one of the features of v5.0.0 which will be released following govuk-frontend 5.

netlify · 2023-11-02T16:58:54Z

✅ Deploy Preview for govuk-components ready!

Name	Link
🔨 Latest commit	`ceba1d0`
🔍 Latest deploy log	https://app.netlify.com/sites/govuk-components/deploys/6563817a5d58540008bc6bb5
😎 Deploy Preview	https://deploy-preview-419--govuk-components.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

peteryates · 2023-11-17T11:29:12Z

@frankieroberto - in terms of reviewing this, the diff will be confusing because the change moves the old link helpers to a different file and adds new implementations in their place.

The method names are the same so if people want to use the originals they can manually include the module, but the new ones will be default. The code is much cleaner in the new ones so they should be easy to review on their own.

In making this change we lose the old Rails way of creating links, but that's actively discouraged by the Rails docs and you don't really see it any more (although we do have some in ECF!)

frankieroberto · 2023-11-17T13:26:11Z

@peteryates thanks, I'll try to review! Would you mind updating the PR description to show what the old and new link syntax looks like? That might make it a bit clearer.

peteryates · 2023-11-17T13:48:37Z

Have done, and here's a comparison of old vs new:

# old
link_to("Show profile", controller: "profiles", action: "show", id: @profile)

# new (restful)
link_to("Show profile", profile_path(@profile))

frankieroberto · 2023-11-17T14:17:37Z

Have done, and here's a comparison of old vs new:

Thanks! I've not seen anyone use the old style for ages! Presumably both examples should be govuk_link_to not link_to?

Will anyone using the old style get an error, or will it just render a link but in an unexpected way?

peteryates · 2023-11-17T14:45:13Z

Presumably both examples should be govuk_link_to not link_to?

I was just showing the Rails defaults when demonstrating the formats, but I've chagnged it to govuk_link_to in the PR description 👍🏽

Will anyone using the old style get an error, or will it just render a link but in an unexpected way?

They'll get an ArgumentError: unknown keyword: :controller or similar.

frankieroberto

Left some comments/suggestions - but feel free to ignore. Looks good!

app/helpers/govuk_link_helper.rb

frankieroberto · 2023-11-17T15:09:02Z

app/helpers/govuk_rails_compatibile_link_helper.rb

+  def govuk_link_classes(*styles, default_class: "#{brand}-link")
+    if (invalid_styles = (styles - link_styles.keys)) && invalid_styles.any?
+      fail(ArgumentError, "invalid styles #{invalid_styles.to_sentence}. Valid styles are #{link_styles.keys.to_sentence}")
+    end
+
+    [default_class] + link_styles.values_at(*styles).compact
+  end
+
+  def govuk_button_classes(*styles, default_class: "#{brand}-button")
+    if (invalid_styles = (styles - button_styles.keys)) && invalid_styles.any?
+      fail(ArgumentError, "invalid styles #{invalid_styles.to_sentence}. Valid styles are #{button_styles.keys.to_sentence}")
+    end
+
+    [default_class] + button_styles.values_at(*styles).compact
+  end


Should these be private methods? Can't think of why you'd use them outside of a link or button helper.

If not, maybe move them below the other methods, and add a brief comment to explain what they do? It threw me a bit seeing these alongside the more obvious helper methods below.

So these are exposed to make it easier for devs to use Rails' other link helpers, like link_to_if and link_to_unless, link_to_unless_current, etc.

frankieroberto · 2023-11-17T15:14:09Z

app/helpers/govuk_rails_compatibile_link_helper.rb

+    extra_options = options if block_given?
+    html_options = build_html_options(extra_options)
+
+    if block_given?
+      link_to(name, html_options, &block)
+    else
+      link_to(name, options, html_options)
+    end


I think you can avoid the if conditions here and always pass block on to the rails helpers, as those do their own if block_given? checks?

Suggested change

extra_options = options if block_given?

html_options = build_html_options(extra_options)

if block_given?

link_to(name, html_options, &block)

else

link_to(name, options, html_options)

end

extra_options = options if block_given?

html_options = build_html_options(extra_options) || nil

link_to(name, options, html_options, &block)

Going to leave these as they're in the legacy implementation which will soon be dropped.

guide/lib/helpers/class_helpers.rb

frankieroberto · 2023-11-17T15:19:14Z

spec/helpers/govuk_rails_compatibile_link_helper_spec.rb

+  describe "#govuk_link_to" do
+    let(:link_text) { 'Onwards!' }
+    let(:link_url) { '/some/link' }
+    let(:link_params) { { controller: :some_controller, action: :some_action } }


Confused by this - thought you’d deprecated this method, and now always require a path to be set instead of setting a controller and action?

This covers the old helpers which are still present (in GovukRailsCompatibileLinkHelper) for teams working on codebases that won't support the new style.

These are clean implementations of that significantly simplify the codebase and allow us to easily support features of the design system, like opening links in new tabs. The new methods are: * govuk_link_to * govuk_button_to * govuk_button_link_to * govuk_link_classes * govuk_button_classes * govuk_breadcrumb_link_to They drop support for Rails' legacy-style link building where the action, controller and id arguments were passed in. Rails discourages their use in the API docs[0]: > Because it relies on url_for, link_to supports both older-style > controller/action/id arguments and newer RESTful routes. Current Rails > style favors RESTful routes whenever possible, so base your application > on resources and use [0] https://api.rubyonrails.org/v7.0/classes/ActionView/Helpers/UrlHelper.html#method-i-link_to

Nanoc's link helpers expect classes to be provided in a string but now we're using arrays internally throughout so it makes sense to use the Rails version.

Now we're using the new link helper we need to use keyword arguments instead of hashes or arrays.

Co-authored-by: Frankie Roberto <frankie@frankieroberto.com>

Following the release of [GOV.UK Design System v5.0](https://github.com/alphagov/govuk-design-system/milestone/16), govuk-components v5.0.0 will be released. The headline features are: * support for GOV.UK Design System v5.0 * inclusion of [the task list component](alphagov/govuk-frontend#2261) #425 * all new link helpers #419 * update header to use new SVG which combines the crown and the GOV.UK test #466 Release notes will be drafted in #469.

peteryates commented May 31, 2023

View reviewed changes

app/helpers/govuk_new_link_helper.rb Outdated Show resolved Hide resolved

peteryates force-pushed the new-link-helpers branch 7 times, most recently from 4ea9703 to b0f9d4e Compare June 11, 2023 19:10

peteryates marked this pull request as ready for review June 11, 2023 19:12

peteryates commented Jun 12, 2023

View reviewed changes

peteryates mentioned this pull request Sep 11, 2023

Add helper: click_govuk_link x-govuk/govuk-rspec-helpers#6

Merged

peteryates added the v5.0.0 label Sep 11, 2023

peteryates force-pushed the new-link-helpers branch from 640125c to 4c0eaca Compare November 2, 2023 16:58

peteryates mentioned this pull request Nov 16, 2023

Version 5.0.0 #456

Merged

frankieroberto approved these changes Nov 17, 2023

View reviewed changes

peteryates added 3 commits November 24, 2023 12:02

Replace Nanoc's LinkTo with Rails' UrlHelper

e925f0a

Nanoc's link helpers expect classes to be provided in a string but now we're using arrays internally throughout so it makes sense to use the Rails version.

Update guide link examples to use keyword args

3a4ec34

Now we're using the new link helper we need to use keyword arguments instead of hashes or arrays.

Reword error messages for bad class combinations

ceba1d0

Co-authored-by: Frankie Roberto <frankie@frankieroberto.com>

peteryates force-pushed the new-link-helpers branch from 4c0eaca to ceba1d0 Compare November 26, 2023 17:33

peteryates added this pull request to the merge queue Nov 26, 2023

Merged via the queue into main with commit aed6a5e Nov 26, 2023
13 checks passed

peteryates deleted the new-link-helpers branch November 26, 2023 17:52

peteryates mentioned this pull request Nov 28, 2023

Version 5 release notes #469

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New link helpers #419

New link helpers #419

peteryates commented May 29, 2023 •

edited

Loading

netlify bot commented May 29, 2023 •

edited

Loading

frankieroberto commented Jun 12, 2023

peteryates commented Jun 12, 2023

peteryates Jun 12, 2023 •

edited

Loading

frankieroberto commented Jun 12, 2023

peteryates commented Jun 13, 2023

peteryates commented Jul 17, 2023

netlify bot commented Nov 2, 2023 •

edited

Loading

peteryates commented Nov 17, 2023

frankieroberto commented Nov 17, 2023

peteryates commented Nov 17, 2023

frankieroberto commented Nov 17, 2023

peteryates commented Nov 17, 2023

frankieroberto left a comment

frankieroberto Nov 17, 2023

peteryates Nov 24, 2023

frankieroberto Nov 17, 2023

peteryates Nov 26, 2023

frankieroberto Nov 17, 2023

peteryates Nov 24, 2023

New link helpers #419

New link helpers #419

Conversation

peteryates commented May 29, 2023 • edited Loading

Multiple versions of link helpers

Final things to do

netlify bot commented May 29, 2023 • edited Loading

✅ Deploy Preview for govuk-components ready!

frankieroberto commented Jun 12, 2023

peteryates commented Jun 12, 2023

peteryates Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

frankieroberto commented Jun 12, 2023

peteryates commented Jun 13, 2023

peteryates commented Jul 17, 2023

netlify bot commented Nov 2, 2023 • edited Loading

✅ Deploy Preview for govuk-components ready!

peteryates commented Nov 17, 2023

frankieroberto commented Nov 17, 2023

peteryates commented Nov 17, 2023

frankieroberto commented Nov 17, 2023

peteryates commented Nov 17, 2023

frankieroberto left a comment

Choose a reason for hiding this comment

frankieroberto Nov 17, 2023

Choose a reason for hiding this comment

peteryates Nov 24, 2023

Choose a reason for hiding this comment

frankieroberto Nov 17, 2023

Choose a reason for hiding this comment

peteryates Nov 26, 2023

Choose a reason for hiding this comment

frankieroberto Nov 17, 2023

Choose a reason for hiding this comment

peteryates Nov 24, 2023

Choose a reason for hiding this comment

peteryates commented May 29, 2023 •

edited

Loading

netlify bot commented May 29, 2023 •

edited

Loading

peteryates Jun 12, 2023 •

edited

Loading

netlify bot commented Nov 2, 2023 •

edited

Loading