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

SAA-354: Package frontend assets #113

Merged
merged 12 commits into from
Feb 13, 2023

Conversation

smcveigh941
Copy link
Contributor

No description provided.

@smcveigh941 smcveigh941 marked this pull request as draft February 9, 2023 23:02
gulpfile.js Outdated
Comment on lines 1 to 6
const gulp = require('gulp')
const concat = require('gulp-concat')
const minify = require('gulp-minify')
const umd = require('gulp-umd')
const merge = require('merge-stream')
const sass = require('gulp-sass')(require('sass'))

Choose a reason for hiding this comment

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

Not strongly opposed to the use of gulp, but in case you weren't aware GDS have decided to move away from gulp for their frontend builds, alphagov/govuk-frontend#2717

Have other options been considered here? Gulp hasn't been updated since 2019 and has fallen out of popularity in recent years.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at using grunt instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with Grunt

gulpfile.js Outdated
Comment on lines 60 to 61
'node_modules/jquery/dist/jquery.js',
'node_modules/jquery-ui-dist/jquery-ui.js',

Choose a reason for hiding this comment

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

It looks like everywhere JQuery is used it can be replaced with vanilla JS?

The only exception seems to be the datepicker, however that's to be removed and seems to have accessible issues? I believe the datepicker is also the only place JQuery UI is being used?

Is JQuery / JQuery UI needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea also thinking to remove it

gulpfile.js Outdated
Comment on lines 40 to 45
exports: function () {
return 'ActivitiesFrontend'
},
namespace: function () {
return 'ActivitiesFrontend'
},

Choose a reason for hiding this comment

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

Suggested change
exports: function () {
return 'ActivitiesFrontend'
},
namespace: function () {
return 'ActivitiesFrontend'
},
exports: () => 'ActivitiesFrontend',
namespace: () => 'ActivitiesFrontend',

@smcveigh941 smcveigh941 marked this pull request as ready for review February 10, 2023 15:46
@smcveigh941 smcveigh941 force-pushed the dev/SAA-354-modularise-frontend-assets branch 2 times, most recently from beb8830 to 12e79d3 Compare February 12, 2023 18:49
@@ -0,0 +1,22 @@
//TODO: These should be removed eventually
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we keeping these if they are all commented out anyway?

Copy link
Contributor Author

@smcveigh941 smcveigh941 Feb 13, 2023

Choose a reason for hiding this comment

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

They are part of the spike so will be removed, but also did not want to spend time fixing up the JS to be compatible with this new format so just commented out ... (I am unsure if we need to keep it right now for reference, but pretty sure we wont be doing Jquery driven pages)

Copy link
Contributor

@matt-crowson-moj matt-crowson-moj left a comment

Choose a reason for hiding this comment

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

LGTM

@smcveigh941 smcveigh941 merged commit 67b5716 into main Feb 13, 2023
@smcveigh941 smcveigh941 deleted the dev/SAA-354-modularise-frontend-assets branch February 13, 2023 13:00
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