alphagov / tech-docs-gem

Gem to distribute the tech docs project
https://tdt-documentation.london.cloudapps.digital/
MIT License
15 stars 38 forks source link

Make GOV.UK Frontend assets paths work with http prefix #292

Open lfdebrux opened 2 years ago

lfdebrux commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.