Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lfdebrux
Copy link
Member

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.

Copy link
Member

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

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!

@lfdebrux lfdebrux changed the title Add code to automatically prefix govuk-frontend assets path with http prefix Make GOV.UK Frontend assets path work with http prefix Aug 17, 2022
@lfdebrux lfdebrux changed the title Make GOV.UK Frontend assets path work with http prefix Make GOV.UK Frontend assets paths work with http prefix Aug 17, 2022
@lfdebrux lfdebrux force-pushed the ldeb-govuk-frontend-http-prefix branch from e6bf158 to 9c4794a Compare August 17, 2022 14:11
@lfdebrux
Copy link
Member Author

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.

Copy link
Member

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

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.

lib/assets/stylesheets/_govuk_tech_docs.scss Outdated Show resolved Hide resolved
$govuk-font-url-function: 'tech-docs-font-url-function';
$govuk-image-url-function: 'tech-docs-image-url-function';

$govuk-assets-path: "/assets/govuk/assets/";
Copy link
Member

Choose a reason for hiding this comment

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

I suspect this does work as intended, because presumably $govuk-fonts-path isn't evaluated until the function is called, but it might make people's heads hurt less if this variable was set before the functions were defined

(I appreciate that $govuk-images-path and $govuk-fonts-path haven't been defined at all at this point!)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, yes, I don't know, I kind of thought that having it after would help people realise that $govuk-{images,fonts}-path are not defined here and that they need to dig deeper to understand it better. Maybe a comment would be more kind though 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added some comments, let me know if you think more detail is needed.

@lfdebrux
Copy link
Member Author

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 and others added 3 commits August 17, 2022 15:31
Co-authored-by: Oliver Byford <oliver.byford@digital.cabinet-office.gov.uk>
Setting govuk_assets_path should no longer be necessary, and to keep the
code simple it will no longer have any effect. This commit lets users know
about this change in case it causes them some surprise.
@lfdebrux lfdebrux force-pushed the ldeb-govuk-frontend-http-prefix branch from 9c4794a to 2420f66 Compare August 17, 2022 14:31
@laurahiles laurahiles moved this from Review to In progress in Technical Documentation Template project board Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants