tech-docs-gem icon indicating copy to clipboard operation
tech-docs-gem copied to clipboard

Make GOV.UK Frontend assets paths work with http prefix

Open lfdebrux opened this issue 3 years ago • 2 comments

What’s changed

You no longer need to change the Sass variable govuk-assets-path when using a http prefix; the tech docs gem will add the prefix for you.

Identifying a user need

Users who want to deploy their tech docs to GitHub pages need to handle an additional path prefix to all URLs; they may want to use the http_prefix setting for this. PR #197 added this http prefix to all assets from the tech docs gem, but required the user to also override govuk-assets-path themselves so that assets from GOV.UK Frontend had the correct path. This PR should mean that this additional step is no longer required, making configuration easier for users.

lfdebrux avatar Jan 25 '22 08:01 lfdebrux

If I understand what this is doing correctly, then I think there's a neater way to do this that doesn't need us to monkey patch the asset_path function.

GOV.UK Frontend allows us to define which function to use when generating paths for images ($govuk-image-url-function) and the font ($govuk-font-url-function).

If no custom function is set, the default behaviour is just to prefix the $filename with $govuk-images-path / $govuk-fonts-path (which default to "#{$govuk-assets-path}images/" and "#{$govuk-assets-path}fonts/" respectively).

I think we can instead pass our own function instead which does the same thing but then calls asset-path with the resolved path including the filename. So something like this:

$govuk-assets-path: "/assets/govuk/assets/";

@function tech-docs-image-handler($filename) {
  @return asset-path($govuk-image-url-function + $filename);
}

$govuk-image-url-function: 'tech-docs-image-handler';

As ever, happy to talk through it if it's helpful!

Thanks for this @36degrees, this is much nicer!

I've pushed a different version of the code now based on your suggestion.

lfdebrux avatar Aug 17 '22 14:08 lfdebrux

I'm guessing there's no easy way to test this, given the functionality we're interested in is really part of Sprockets?

Otherwise I'm happy with this approach, bar a couple of very minor comments.

Ah tests is a good idea, I'll add some.

lfdebrux avatar Aug 17 '22 14:08 lfdebrux