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

Add CSS hot-reloading #2050

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ coverage: venv ## Create coverage report

.PHONY: run-dev
run-dev:
poetry run flask run -p 6012 --host=localhost
npm run watch & \
poetry run flask run -p 6012 --host=localhost

.PHONY: run-gunicorn
run-gunicorn:
Expand Down
1 change: 0 additions & 1 deletion app/assets/javascripts/branding_request.min.js

This file was deleted.

1 change: 0 additions & 1 deletion app/assets/javascripts/formValidateRequired.min.js

This file was deleted.

2 changes: 2 additions & 0 deletions app/assets/javascripts/index.min.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion app/assets/javascripts/main.min.js

Large diffs are not rendered by default.

1 change: 0 additions & 1 deletion app/assets/javascripts/sessionRedirect.min.js

This file was deleted.

1 change: 0 additions & 1 deletion app/assets/javascripts/templateFilters.min.js

This file was deleted.

1 change: 0 additions & 1 deletion app/assets/javascripts/touDialog.min.js

This file was deleted.

93 changes: 92 additions & 1 deletion app/assets/stylesheets/index.css

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions app/assets/stylesheets/tailwind/style.css
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@

@import "./toolkit/wp.css";

@import "../fa-svg-with-js.css";
@import "../../../../node_modules/accessible-autocomplete/dist/accessible-autocomplete.min.css";

/*! purgecss start ignore */
@-ms-viewport {
width: device-width;
Expand Down
2 changes: 1 addition & 1 deletion app/templates/main_template.html
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@
{% include 'partials/google-analytics.html' %}

{% block page_script %}
<script nonce="{{ request_nonce }}" type="text/javascript" src="{{ asset_url('javascripts/main.min.js') }}"></script>
<script nonce="{{ request_nonce }}" type="text/javascript" src="{{ asset_url('javascripts/index.min.js') }}"></script>
<script nonce="{{ request_nonce }}" type="text/javascript" src="{{ asset_url('javascripts/all.min.js') }}"></script>
Comment on lines +170 to 171
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to load both "index" and "all"? It seems like we prepend all with index in gulpfile.

Copy link
Member Author

Choose a reason for hiding this comment

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

We dont combine them - but i have no idea why. I think we probably could just have the one.


{% if current_user.is_authenticated %}
Expand Down
2 changes: 1 addition & 1 deletion app/templates/notify_template.html
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
{% include 'partials/cds_ascii_art.html' %}
{% include 'partials/google-tag-manager-body.html' %}
{% block main_content %}{% endblock %}
<script nonce="{{ request_nonce }}" type="text/javascript" src="{{ asset_url('javascripts/main.min.js') }}"></script>
<script nonce="{{ request_nonce }}" type="text/javascript" src="{{ asset_url('javascripts/index.min.js') }}"></script>
<script nonce="{{ request_nonce }}" type="text/javascript" src="{{ asset_url('javascripts/all.min.js') }}"></script>
</body>
</html>
47 changes: 28 additions & 19 deletions gulpfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ plugins.addSrc = require("gulp-add-src");
plugins.babel = require("gulp-babel");
plugins.base64 = require("gulp-base64-inline");
plugins.concat = require("gulp-concat");
plugins.faMinify = require("gulp-fa-minify");
plugins.cleanCSS = require('gulp-clean-css');
plugins.jshint = require("gulp-jshint");
plugins.prettyerror = require("gulp-prettyerror");
plugins.rename = require("gulp-rename");
//plugins.uglify = require("gulp-uglify");
plugins.uglify = require("gulp-uglify");

// 2. CONFIGURATION
// - - - - - - - - - - - - - - -
Expand Down Expand Up @@ -86,35 +86,44 @@ const javascripts = () => {
"accessible-autocomplete/dist/accessible-autocomplete.min.js",
])
)
//.pipe(plugins.uglify())
.pipe(plugins.uglify())
.pipe(plugins.concat("all.min.js"))
.pipe(
plugins.addSrc.prepend([
paths.src + "javascripts/main.min.js",
paths.src + "javascripts/index.min.js",
paths.src + "javascripts/scheduler.min.js",
paths.src + "javascripts/branding_request.min.js",
paths.src + "javascripts/formValidateRequired.min.js",
paths.src + "javascripts/sessionRedirect.min.js",
paths.src + "javascripts/touDialog.min.js",
paths.src + "javascripts/templateFilters.min.js",
])
)
.pipe(dest(paths.dist + "javascripts/"));
};

const minifyIndividualJs = () => {
return src([
paths.src + "javascripts/branding_request.js",
paths.src + "javascripts/formValidateRequired.js",
paths.src + "javascripts/sessionRedirect.js",
paths.src + "javascripts/touDialog.js",
paths.src + "javascripts/templateFilters.js"
Comment on lines +102 to +106
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these ever bundled? Do we load them piece by piece in our html templates? If not, we could remove this step?

Copy link
Member Author

Choose a reason for hiding this comment

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

These few are not bundled. The thought when they were created was that since they are not often used, they could exist separately. Although, the templateFilters one probably should be bundled now that I look more closely.

])
.pipe(plugins.prettyerror())
.pipe(plugins.babel({
presets: ["@babel/preset-env"]
}))
.pipe(plugins.uglify())
.pipe(plugins.rename({ suffix: '.min' }))
.pipe(dest(paths.dist + "javascripts/"));
};

// copy static css
const static_css = () => {
return src(paths.src + "/stylesheets/index.css")
.pipe(
plugins.addSrc.prepend([
paths.npm + "accessible-autocomplete/dist/accessible-autocomplete.min.css",
paths.src + "stylesheets/fa-svg-with-js.css",
])
)
.pipe(plugins.concat("index.css"))
.pipe(
dest(paths.dist + "stylesheets/")
);
.pipe(plugins.cleanCSS({
level: 2,
}, (details) => {
console.log(`${details.name}: Original size:${details.stats.originalSize} - Minified size: ${details.stats.minifiedSize}`)
amazingphilippe marked this conversation as resolved.
Show resolved Hide resolved
}))
.pipe(dest(paths.dist + "stylesheets/"));
};

// Copy images
Expand Down Expand Up @@ -144,7 +153,7 @@ const watchFiles = {

// Default: compile everything
const defaultTask = parallel(
series(javascripts),
series(minifyIndividualJs, javascripts),
series(images),
series(static_css),
);
Expand Down
34 changes: 34 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@
"test": "jest --config tests/javascripts/jest.config.js tests/javascripts",
"webpack": "webpack --config webpack.config.js",
"build": "gulp",
"watch": "gulp watch",
"watch": "npx tailwindcss -i ./app/assets/stylesheets/tailwind/style.css -o ./app/static/stylesheets/index.css --watch ",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need gulp in there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope - all the css is within styles.css now and we shouldnt add arbitrary css to gulp anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

So watch will only update on css changes, and not script changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah - I didnt tackle watching the js - could be another PR!

"heroku-postbuild": "gulp && make generate-version-file && export ADMIN_BASE_URL=${HEROKU_APP_NAME}.herokuapp.com",
"tailwind": "webpack --config webpack.config.js && gulp"
"tailwind": "npx tailwindcss -i ./app/assets/stylesheets/tailwind/style.css -o ./app/assets/stylesheets/index.css --minify && webpack --config webpack.config.js && gulp"
},
"repository": {
"type": "git",
Expand Down Expand Up @@ -57,6 +57,7 @@
"gulp-add-src": "1.0.0",
"gulp-babel": "8.0.0",
"gulp-base64-inline": "1.0.6",
"gulp-clean-css": "^4.3.0",
"gulp-concat": "2.6.1",
"gulp-fa-minify": "^6.0.1",
"gulp-include": "2.4.1",
Expand Down
5 changes: 4 additions & 1 deletion tailwind.config.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
const plugin = require("tailwindcss/plugin");

module.exports = {
content: ["./app/**/*.{html,css,js}"],
content: [
"./app/**/*.{html,css,js}",
"./app/assets/javascripts/fontawesome.js"
],
theme: {
container: {
center: true,
Expand Down
2 changes: 1 addition & 1 deletion tests/app/main/views/test_headers.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ def test_owasp_useful_headers_set(client, mocker, mock_get_service_and_organisat
"public, max-age=31536000, immutable",
),
(
"javascripts/main.min.js",
"javascripts/index.min.js",
True,
"public, max-age=31536000, immutable",
),
Expand Down
18 changes: 1 addition & 17 deletions webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,7 @@ module.exports = {
// mode: "development", //development
mode: "production",
entry: {
main: ["./app/assets/javascripts/index.js", "./app/assets/stylesheets/tailwind/style.css"],
branding_request: ["./app/assets/javascripts/branding_request.js"],
formValidateRequired: ["./app/assets/javascripts/formValidateRequired.js"],
sessionRedirect: ["./app/assets/javascripts/sessionRedirect.js"],
touDialog: ["./app/assets/javascripts/touDialog.js"],
templateFilters: ["./app/assets/javascripts/templateFilters.js"],
index: ["./app/assets/javascripts/index.js"],
Copy link
Contributor

Choose a reason for hiding this comment

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

lovely

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this file being named main.js but having a source of index.js when we ALREADY HAVE a file named main.js was just so super confusing.

This is still confusing - but less so :D

scheduler: {
import: './app/assets/javascripts/scheduler/scheduler.js',
library: {
Expand Down Expand Up @@ -44,17 +39,6 @@ module.exports = {
],
use: ["style-loader", "css-loader"]
},
{
test: /\.css$/i,
include: [
path.resolve(__dirname, "app/assets/stylesheets/tailwind")
],
use: [
MiniCssExtractPlugin.loader,
"css-loader",
"postcss-loader"
]
},
Comment on lines -47 to -57
Copy link
Contributor

Choose a reason for hiding this comment

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

Good riddance!

{ test: /\.(png|svg|jpg|gif|ico)$/, use: ["file-loader"] },
{
test: /.(js|jsx)$/,
Expand Down
Loading