Skip to content
This repository has been archived by the owner on Feb 17, 2021. It is now read-only.

Put JS through build pipeline #285

Merged
merged 7 commits into from
Oct 28, 2016
Merged

Put JS through build pipeline #285

merged 7 commits into from
Oct 28, 2016

Conversation

callumlocke
Copy link
Contributor

No description provided.

@joannaskao joannaskao temporarily deployed to ft-ig-us-elections-poll-pr-285 October 27, 2016 16:29 Inactive
@joannaskao joannaskao temporarily deployed to ft-ig-us-elections-poll-pr-285 October 27, 2016 18:04 Inactive
@callumlocke callumlocke requested a deployment to ft-ig-us-elections-poll-pr-285 October 27, 2016 18:04 Pending
@callumlocke
Copy link
Contributor Author

fixes #286

@callumlocke callumlocke reopened this Oct 27, 2016
@joannaskao joannaskao temporarily deployed to ft-ig-us-elections-poll-pr-285 October 27, 2016 18:04 Inactive
@joannaskao joannaskao temporarily deployed to ft-ig-us-elections-poll-pr-285 October 27, 2016 18:11 Inactive
Copy link
Member

@kavanagh kavanagh left a comment

Choose a reason for hiding this comment

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

Do not merge. This will put the page behind the paywall.

@@ -1,4 +1,7 @@
@import 'o-grid/main';
@import 'o-fonts/main';

@include oFontsInclude(MetricWeb, medium);
Copy link
Member

Choose a reason for hiding this comment

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

Don't. We don't need every font. I'll change the font weight CSS 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.

We do need this font. It's used all over the place. The Semibold is only used in one place, we should remove that if we need to remove one.

Copy link
Member

Choose a reason for hiding this comment

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

Things should be changed to semi bold. I you remember, this change happen during the run up to Brexit too. We've limited the font set to keep it as close as possible to the rest of the site.

@@ -10,7 +10,7 @@
]
%}
{% set scripts = [
'main.js',
'main.entry.js',
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

@callumlocke callumlocke Oct 27, 2016

Choose a reason for hiding this comment

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

The browserify default is for *.entry.js to be treated as an entry. I could rename the file back to main.js and just configure browserify to treat main.js as an entry instead, if you prefer

Copy link
Member

Choose a reason for hiding this comment

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

Fine.

const port = server.address().port;
console.log(`running ${host} ${port}`);
console.log(`running http://localhost:${port}/`);
Copy link
Member

Choose a reason for hiding this comment

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

What about prod. That's not local host.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@@ -100,7 +98,7 @@ app.get('/__access_metadata', (req, res) => {
res.setHeader('Content-Type', 'application/json');
res.setHeader('Cache-Control', `public, max-age=86400`);
res.send(`
{"access_metadata":[{"path_regex":"\/us-elections*","classification":"unconditional"},
{"access_metadata":[{"path_regex":"/us-elections*","classification":"unconditional"},
Copy link
Member

Choose a reason for hiding this comment

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

The slash is needed. Do not merge this!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah the linter complained about it as unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that backslash is supposed to be in the output, it should be \\.

  • \\/ translates to \/
  • \/ translates to /
  • / translates to /

which do we want

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I need to check.

Copy link
Member

Choose a reason for hiding this comment

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

👍 ok, just needed to also check the access service docs

@callumlocke callumlocke temporarily deployed to ft-ig-us-elections-poll-pr-285 October 28, 2016 09:51 Inactive
@callumlocke callumlocke temporarily deployed to ft-ig-us-elections-poll-pr-285 October 28, 2016 10:00 Inactive
@callumlocke callumlocke temporarily deployed to ft-ig-us-elections-poll-pr-285 October 28, 2016 10:11 Inactive
@callumlocke callumlocke merged commit 003c2a8 into master Oct 28, 2016
@callumlocke callumlocke deleted the prebuild-javascript branch October 28, 2016 10:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants