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

Enhancement/template generator #59

Merged
merged 2 commits into from Jan 22, 2017

Conversation

Projects
None yet
3 participants
@butlerx
Copy link
Contributor

commented Jan 22, 2017

close #21

@butlerx butlerx force-pushed the butlerx:enhancement/template-generator branch from b68c4b3 to 03b8441 Jan 22, 2017

.travis.yml Outdated
@@ -1,9 +0,0 @@
language: node_js

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Jan 22, 2017

Contributor

I believe this change is being made in #58

@@ -1,34 +1,37 @@
# TechWeek Website [![Build Status](https://travis-ci.org/redbrick/techweek.dcu.ie.svg?branch=master)](https://travis-ci.org/redbrick/techweek.dcu.ie)
# TechWeek Website [![CircleCI](https://circleci.com/gh/redbrick/techweek.dcu.ie/tree/master.svg?style=svg)](https://circleci.com/gh/redbrick/techweek.dcu.ie/tree/master)

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Jan 22, 2017

Contributor

Can you move it to another PR? This should be added in #58 but I didn't realize it at the time.

function render (source, events, url) {
let template = Handlebars.compile(source);
let output = template(events);
if (!fs.existsSync(__dirname + '/../dist')) { fs.mkdirSync(__dirname + '/../dist'); }

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Jan 22, 2017

Contributor

Can you move fs.mkdirSync to a new line?

This comment has been minimized.

Copy link
@butlerx

butlerx Jan 22, 2017

Author Contributor

yes but id rather not


function render (source, events, url) {
let template = Handlebars.compile(source);
let output = template(events);

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Jan 22, 2017

Contributor

I think inlining this would be neater as template its not used anywhere else.

const output = (Handlebars.compile(source))(events);

This comment has been minimized.

Copy link
@butlerx

butlerx Jan 22, 2017

Author Contributor

const would break it

const events = require(__dirname + '/../events.json');
const template = fs.readFileSync(__dirname + '/../template.handlebars', "utf-8");

function render (source, events, url) {

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Jan 22, 2017

Contributor

Can you add documentation to this function? What do source, events and url mean in this case.

if (!fs.existsSync(__dirname + '/../dist')) { fs.mkdirSync(__dirname + '/../dist'); }
dir = __dirname + '/../dist';
if (url) {
dir = dir + '/' + url;

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Jan 22, 2017

Contributor

Please use path.join.

fs.mkdirSync(dir);
}
}
fs.writeFile(dir + '/index.html', output, 'utf8', (err) => { if (err) throw err; });

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Jan 22, 2017

Contributor

Move if statement to a new line?

This comment has been minimized.

Copy link
@butlerx

butlerx Jan 22, 2017

Author Contributor

considering its an error check id prefer it to be one line

circle.yml Outdated
@@ -0,0 +1,3 @@
machine:

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Jan 22, 2017

Contributor

This was added in #58. Please remove.

"end": 7,
"month": "March",
"year": 2014,
"live": "2014-03-03T12:00:00",

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Jan 22, 2017

Contributor

Would Unix timestamp be more appropiate? I am not sure is this representation understandable by languages other than JS.

This comment has been minimized.

Copy link
@butlerx

butlerx Jan 22, 2017

Author Contributor

id prefer the timestamp to be human readable

host: 'techweek.dev',
livereload: true
});
});


gulp.task('fonts', function() {
return gulp.src('./node_modules/materialize-css/fonts/**')
.pipe(gulp.dest('dist/fonts'));
return gulp.src('./node_modules/materialize-css/fonts/**')

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Jan 22, 2017

Contributor

I should probably make a new issue for this, would moving to https://github.com/material-components/material-components-web be beneficial seeing as its the (yet another) official Material Design framework?

This comment has been minimized.

Copy link
@butlerx

butlerx Jan 22, 2017

Author Contributor

create a new issue for that, would also be good to remove jquery as part of that

This comment has been minimized.

Copy link
@butlerx

butlerx Jan 22, 2017

Author Contributor

issue #60 opened for this

@@ -1,183 +0,0 @@
[{

This comment has been minimized.

Copy link
@SeanHealy33

SeanHealy33 Jan 22, 2017

Contributor

I'm starting to think the json of the site should probably be in a separate repo since it adds a decent bloat to the diff

This comment has been minimized.

Copy link
@butlerx

butlerx Jan 22, 2017

Author Contributor

yeah the json was originally never really meant to be part of the repo so moving it to a submoule would proabbly be a good idea

@@ -0,0 +1,74 @@
<!DOCTYPE html>

This comment has been minimized.

Copy link
@SeanHealy33

SeanHealy33 Jan 22, 2017

Contributor

😍 nice refactor 👍

@butlerx butlerx merged commit c23f17f into redbrick:master Jan 22, 2017

@butlerx butlerx deleted the butlerx:enhancement/template-generator branch Jan 22, 2017

@butlerx butlerx referenced this pull request Jan 22, 2017

Closed

migrate historic data #31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.