Skip to content

[DO NOT MERGE] WIP #4748

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

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from
Draft

[DO NOT MERGE] WIP #4748

wants to merge 23 commits into from

Conversation

andysellick
Copy link
Contributor

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

What

Why

Visual changes

Trello card: https://trello.com/c/zVqSZM6D

- for now, limit to non prod environment
@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4748 April 8, 2025 08:40 Inactive
@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4748 April 8, 2025 08:42 Inactive
@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4748 April 8, 2025 08:44 Inactive
@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4748 April 8, 2025 08:47 Inactive
@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4748 April 8, 2025 09:09 Inactive
@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4748 April 9, 2025 10:33 Inactive
@AshGDS AshGDS force-pushed the progress-prototype branch from ff03aad to 7f1ba15 Compare April 9, 2025 11:35
@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4748 April 9, 2025 11:35 Inactive
@AshGDS AshGDS force-pushed the progress-prototype branch from 7f1ba15 to db57a84 Compare April 9, 2025 11:38
@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4748 April 9, 2025 11:38 Inactive
@AshGDS AshGDS force-pushed the progress-prototype branch from db57a84 to 1143981 Compare April 9, 2025 13:15
@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4748 April 9, 2025 13:15 Inactive
@AshGDS AshGDS force-pushed the progress-prototype branch from 1143981 to d4ee288 Compare April 9, 2025 15:27
@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4748 April 9, 2025 15:28 Inactive
@AshGDS
Copy link
Contributor

AshGDS commented Apr 9, 2025

@andysellick I've got it working without any Linting/CI/CSP errors anymore - bundle exec rake should pass locally now. The PR will still fail as the data is missing in GitHub. The commits should hopefully explain everything I've changed. If you run the code now you'll need to change your data files to have window.GOVUK = window.GOVUK || {} and then define them on window.GOVUK. to use them now, as Jasmine didn't like const.

One of the main things that took me a while to figure out were the remaining Content Security Policy issues but I managed to get there in the end. I've also made it so the Content Security Policy only applies to this specific Controller (saw they did this in govuk-chat so copied it from there) so that removes the need to update govuk_app_config.

@AshGDS
Copy link
Contributor

AshGDS commented Apr 10, 2025

Going to try and see if I can use this instead of unsafe-inline as that seems to be more secure.

@andysellick
Copy link
Contributor Author

Nice work @AshGDS 👍

@AshGDS AshGDS force-pushed the progress-prototype branch from d4ee288 to 29b222d Compare April 10, 2025 13:52
@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4748 April 10, 2025 13:53 Inactive
@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4748 April 15, 2025 10:16 Inactive
@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4748 April 15, 2025 13:49 Inactive
@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4748 April 16, 2025 13:27 Inactive
@govuk-ci govuk-ci had a problem deploying to govuk-frontend-app-pr-4748 April 16, 2025 13:45 Failure
@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4748 April 16, 2025 14:13 Inactive
@AshGDS AshGDS force-pushed the progress-prototype branch from 7ac4acd to 0ea5407 Compare April 17, 2025 09:34
@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4748 April 17, 2025 09:34 Inactive
andysellick and others added 21 commits April 17, 2025 10:37
- remove fetches, as data is local
- remove omnivore, as CSV data has been converted to geojson (and omnivore was causing an error with our CSP)
- remove Promise, as fetches no longer required
- reindent
I don't think it's worth importing a font just for some branding, so allow it to use the fallback font instead.
Taken from https://github.com/OrdnanceSurvey/os-api-branding/tree/main/img
 and https://github.com/Leaflet/Leaflet/tree/main/dist/images - I have run the SVGs through a compression tool as they were 58KB each, which felt excessive.
Jasmine was trying to import the const files multiple times, which errors as a const can only be defined once. Therefore I've changed it to be on window.GOVUK
Stylelint was run on this previously which helped the spacing/formatting look nicer. However stylelint has 60+ things it can't autofix, which may be too much of a burden atm to fix so disabling this for now.
As I'm not sure how you'd write tests for these.
This stops an unsafe-inline CSP issue.
@AshGDS AshGDS force-pushed the progress-prototype branch from 2a22a0c to dc3f01c Compare April 17, 2025 09:37
@govuk-ci govuk-ci had a problem deploying to govuk-frontend-app-pr-4748 April 17, 2025 09:37 Failure
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.

3 participants