Remove dev styling from compiled css and update pipeline #560

kr8n3r · 2018-02-28T21:42:45Z

We don't want dev specific css in compiled output in dist, as observed in user research.
This PR addresses this by:

creating separate app only sass entry points where $mq-show-breakpoints is included
updates app layout template to reference app*.css files
modifying scss:compile task to use either app*.scss or govuk-frontend*.scss files as source for compilation
adding test to check for presence of that string in compiled stylesheets

Additionally:

removes magic commenting with gulp-replace and gulp-replaces from dependencies
adds a file helper function to read contents of a file for a given path

the flow is now as follows:

if dev then compile app*.scss with mq-show-breapointsand anthing else we might want to include in teh review app
if dist then compile govuk-frontend*.scss without mq-show-breakpoints

This should also address #556

https://trello.com/c/woTnvOTx/801-remove-active-breakpoint-debug-helper-from-dist-css

36degrees · 2018-03-01T08:07:16Z

app/assets/scss/app-old-ie.scss

@@ -0,0 +1,5 @@
+// If you want to display the currently active breakpoint in the top


Is there a reason these aren't using the existing app stylesheets, which are now not being used?

the task to compile css for dist and review app is the same. tried using the same entry point for both and couldn't find a way to not include breakpoints in dist compiled and include them in review compiled
we might also want to add and more styles currently in views into app*.scss

govuk-frontend*.scss files are used for compilation to dist

The govuk-frontend*.scss files in the app folder are used for compilation to dist?

yes. happy to find a better place for them.
they used to be in globals and we were filtering them from copy task.

36degrees · 2018-03-01T08:10:04Z

tasks/gulp/compile-assets.js

@@ -31,9 +30,20 @@ const errorHandler = function (error) {
  this.once('finish', () => process.exit(1))
  this.emit('end')
 }
+// used only for review app as they include showing of media queries


The existing app assets seem to be getting compiled OK, so not sure why this is needed.

36degrees · 2018-03-01T08:11:06Z

tasks/gulp/compile-assets.js

@@ -54,42 +64,6 @@ gulp.task('scss:compile', () => {
      })
    ))
    .pipe(gulp.dest(taskArguments.destination + '/css/'))
-
-  let compileOldIe = gulp.src(configPaths.app + 'assets/scss/govuk-frontend-old-ie.scss')


I don't understand why this is being removed as part of this PR. Don't we still need an old-ie stylesheet?

we still have old ie stylesheet. oldie was not doing anything that we don't currently do already: (flatten mq's, use rems)

Can that change be made in a separate PR? It doesn't seem related to the other changes.

(I think there are some things oldie might be doing or should be doing – patching ::before into :before (which is currently disabled??), changing opacity to filters – but I don't want to hold this PR up)

ok, can leave it in and will add required merge-stream back in

36degrees · 2018-03-01T08:13:05Z

lib/file-helper.js

+
+// Read the contents of a file from a given path
+const readFileContents = filePath => {
+  const contents = fs.readFileSync(filePath, 'utf8')


Can just return fs.readFileSync(filePath, 'utf8')

yeah could do

kr8n3r · 2018-03-01T11:05:58Z

reverted removing oldie compilation

kr8n3r · 2018-03-01T11:16:59Z

@36degrees forgot to add changelog. still ok with it?

36degrees · 2018-03-01T11:33:45Z

LGTM.

igloosi added 4 commits February 28, 2018 17:58

Remove magic commenting

affc508

Remove showing of break points from partial

ce78ab1

Remove gulp-replace

2ee7666

Use app* css files for the review app

d07e191

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-560 February 28, 2018 21:42 Inactive

36degrees reviewed Mar 1, 2018

View reviewed changes

kr8n3r force-pushed the remove-dev-styling-from-compiled-css branch from 0eb3072 to 95cbdc8 Compare March 1, 2018 09:16

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-560 March 1, 2018 09:16 Inactive

igloosi added 3 commits March 1, 2018 11:05

Compile diffrent source files based on task flag

6780570

add test to check build css files in dist

51a1079

Add file helper to read contents of a file

eb6d56b

kr8n3r force-pushed the remove-dev-styling-from-compiled-css branch from 95cbdc8 to eb6d56b Compare March 1, 2018 11:05

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-560 March 1, 2018 11:05 Inactive

36degrees approved these changes Mar 1, 2018

View reviewed changes

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-560 March 1, 2018 11:16 Inactive

Document in changelog

177215a

kr8n3r force-pushed the remove-dev-styling-from-compiled-css branch from 53f7266 to 177215a Compare March 1, 2018 11:17

govuk-design-system-ci deployed to govuk-frontend-review-pr-560 March 1, 2018 11:18 View deployment

kr8n3r merged commit eb936e9 into master Mar 1, 2018

kr8n3r deleted the remove-dev-styling-from-compiled-css branch March 1, 2018 11:37

This was referenced Mar 1, 2018

Release v0.0.24-alpha #564

Closed

Release v0.0.24-alpha #565

Merged

NickColley mentioned this pull request Mar 8, 2018

Change 'devonly' behaviour to avoid magic #556

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dev styling from compiled css and update pipeline #560

Remove dev styling from compiled css and update pipeline #560

kr8n3r commented Feb 28, 2018 •

edited

36degrees Mar 1, 2018

kr8n3r Mar 1, 2018 •

edited

kr8n3r Mar 1, 2018 •

edited

36degrees Mar 1, 2018

kr8n3r Mar 1, 2018

36degrees Mar 1, 2018

36degrees Mar 1, 2018

kr8n3r Mar 1, 2018 •

edited

36degrees Mar 1, 2018

kr8n3r Mar 1, 2018

36degrees Mar 1, 2018

kr8n3r Mar 1, 2018

kr8n3r commented Mar 1, 2018

kr8n3r commented Mar 1, 2018

36degrees commented Mar 1, 2018

		@@ -0,0 +1,5 @@
		// If you want to display the currently active breakpoint in the top

Remove dev styling from compiled css and update pipeline #560

Remove dev styling from compiled css and update pipeline #560

Conversation

kr8n3r commented Feb 28, 2018 • edited

Choose a reason for hiding this comment

kr8n3r Mar 1, 2018 • edited

Choose a reason for hiding this comment

kr8n3r Mar 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kr8n3r Mar 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kr8n3r commented Mar 1, 2018

kr8n3r commented Mar 1, 2018

36degrees commented Mar 1, 2018

kr8n3r commented Feb 28, 2018 •

edited

kr8n3r Mar 1, 2018 •

edited

kr8n3r Mar 1, 2018 •

edited

kr8n3r Mar 1, 2018 •

edited