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

Move shared JavaScript to lib folder #1481

Merged
merged 8 commits into from
Aug 3, 2022
Merged

Move shared JavaScript to lib folder #1481

merged 8 commits into from
Aug 3, 2022

Conversation

lfdebrux
Copy link
Member

@lfdebrux lfdebrux commented Jul 22, 2022

Ideally, a user would just add their javascript to app/assets/javascripts/application.js and not have to see or worry about things like intitialsing govuk-frontend, or the auto-store data JS. This would make updating a user's prototype easier and less risky, because currently it is possible they have changed the standard files. It will also mean less is needed to bootstrap a prototype using a package.

This PR moves the standard JS to the lib folder as lib/assets/javascripts/kit.js, and replaces app/assets/javascripts/application.js with a simple placeholder that a user can rewrite completely as desired.

Note that we make sure that all pages include both scripts by adding kit.js to app/views/includes/scripts.html. This PR doesn't help users update that file, but ideally in another PR we would make it so that updating to v13 also takes care of this.

Resolves #1387.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--5h2aj9 July 22, 2022 15:22 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--5h2aj9 July 22, 2022 15:27 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--5h2aj9 July 22, 2022 15:39 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--5h2aj9 July 22, 2022 16:09 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--5h2aj9 July 22, 2022 16:14 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--5h2aj9 July 25, 2022 10:23 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--5h2aj9 July 25, 2022 10:39 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--5h2aj9 July 25, 2022 16:24 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--5h2aj9 July 26, 2022 08:17 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--5h2aj9 July 26, 2022 08:18 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--5h2aj9 July 26, 2022 08:21 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--5h2aj9 July 27, 2022 09:14 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--5h2aj9 July 27, 2022 09:24 Inactive
@BenSurgisonGDS
Copy link
Contributor

I think the test can be fixed by replacing:

 ` expect.stringMatching(/.*\/VERSION.txt$/),`

with

  `expect.stringMatching(/.*[\/,//]VERSION.txt$/),`

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--gmtxvp August 2, 2022 12:15 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--gmtxvp August 2, 2022 12:21 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--gmtxvp August 2, 2022 14:55 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--gmtxvp August 2, 2022 15:15 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--gmtxvp August 2, 2022 15:23 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--gmtxvp August 2, 2022 15:25 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--gmtxvp August 2, 2022 16:25 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--gmtxvp August 2, 2022 17:04 Inactive
@lfdebrux
Copy link
Member Author

lfdebrux commented Aug 3, 2022

Note that we make sure that all pages include both scripts by adding kit.js to app/views/includes/scripts.html. This PR doesn't help users update that file, but ideally in another PR we would make it so that updating to v13 also takes care of this.

Just wondering if it should be part of this PR, given other changes are being made to the update scripts

As this PR is already pretty big I've put the layout template includes changes in a separate PR, #1499.

lfdebrux and others added 5 commits August 3, 2022 09:26
Also rename application.js to kit.js to avoid clashes.

Co-authored-by: Joe Lanman <joe.lanman@digital.cabinet-office.gov.uk>
Co-authored-by: NoraGDS <57447099+NoraGDS@users.noreply.github.com>
We want to make sure that any shared JS files we create can't have name
collisions with files the user wants to create. Putting the files in a
separate namespaced folder mitigates against this.
The update script tests run create-release-archive a lot, which involves
a lot of shelling out, and on Windows this is very slow, so we increase
the test timeout again to account for this. Ideally there would be less
shelling out, but that's a problem for another day.
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--gmtxvp August 3, 2022 08:29 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--gmtxvp August 3, 2022 08:39 Inactive
We want to make sure that on Windows we can compare files regardless of
what line endings the users files have, and that any changes we make
have the same line endings.
Simplify the update shell script by combining the Node.js post scripts
into one mega script.

We also move the final message from update script to very end.
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-move--gmtxvp August 3, 2022 08:57 Inactive
Copy link
Contributor

@joelanman joelanman left a comment

Choose a reason for hiding this comment

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

great work

@lfdebrux lfdebrux merged commit 5da3de9 into main Aug 3, 2022
@lfdebrux lfdebrux deleted the ldeb-move-js-to-lib branch August 3, 2022 09:33
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.

Simplify update process - application.js
6 participants