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

Get scripts and assets from config #2319

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BenSurgisonGDS
Copy link
Contributor

@BenSurgisonGDS BenSurgisonGDS commented Aug 30, 2023

Remove hard coded references for the locations of the govuk-frontend scripts and assets.

@BenSurgisonGDS BenSurgisonGDS force-pushed the get-scripts-and-assets-from-config branch 2 times, most recently from 4d31fab to d8276fb Compare August 30, 2023 12:47
@BenSurgisonGDS BenSurgisonGDS marked this pull request as ready for review August 30, 2023 13:02
@BenSurgisonGDS BenSurgisonGDS changed the base branch from main to using-internal-govuk-frontend-more-widely August 30, 2023 13:16
Base automatically changed from using-internal-govuk-frontend-more-widely to main August 31, 2023 08:42
@BenSurgisonGDS BenSurgisonGDS force-pushed the get-scripts-and-assets-from-config branch from 5895226 to 03bd181 Compare September 11, 2023 10:17
@@ -74,6 +74,11 @@ module.exports = function setupNodeEvents (on, config) {
config.env.packageFolder = path.join(config.env.projectFolder, 'node_modules', 'govuk-prototype-kit')
}

if ('govuk-frontend' in dependencies) {
const frontEndConfigFile = path.join(config.env.projectFolder, 'node_modules', 'govuk-frontend', 'govuk-prototype-kit.config.json')
config.env.frontendAssetsFolder = fse.readJsonSync(frontEndConfigFile).assets.find(asset => !asset.split(path.sep).pop().includes('.'))
Copy link
Contributor

Choose a reason for hiding this comment

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

This closely targets the current implementation inside govuk-frontend - if they add another assets item to their config which doesn't have any dots then this may identify the wrong one.

<script src="/manage-prototype/dependencies/govuk-frontend/govuk/all.js"></script>
<script src="/manage-prototype/dependencies/govuk-frontend/govuk-prototype-kit/init.js"></script>
<script src="/plugin-assets/govuk-prototype-kit/lib/assets/javascripts/kit.js"></script>
{% for scriptConfig in managePlugins.scripts %}
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be a duplicate of govuk-prototype-kit/includes/scripts.njk, and both are taking their input from the plugin config. We should reuse the existing include rather than copying it. This can be done by calling the field in the model pluginConfig rather than managePlugins.

@BenSurgisonGDS BenSurgisonGDS force-pushed the get-scripts-and-assets-from-config branch 2 times, most recently from eb696aa to 532b418 Compare September 28, 2023 13:41
@BenSurgisonGDS BenSurgisonGDS force-pushed the get-scripts-and-assets-from-config branch from 532b418 to 9c11033 Compare September 28, 2023 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants