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 support for multiple templates to allow for addition of call for talks page #64

Merged
merged 1 commit into from Mar 5, 2017

Conversation

Projects
None yet
3 participants
@butlerx
Copy link
Contributor

commented Mar 3, 2017

use front matter for parsing json to add support for md

and uopdate tests for this

@butlerx butlerx force-pushed the butlerx:enhancmentz/CFT branch 4 times, most recently from e3cb17c to 36e4c16 Mar 3, 2017

@butlerx butlerx requested review from SeanHealy33 and VoyTechnology Mar 4, 2017

@SeanHealy33

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2017

That is the longest 😨 title ever

@@ -0,0 +1,115 @@
{{{

This comment has been minimized.

Copy link
@SeanHealy33

SeanHealy33 Mar 5, 2017

Contributor

map map map?

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Mar 5, 2017

Contributor

Template i presume. But if so i would rename the file from .json to .tmpl

This comment has been minimized.

Copy link
@butlerx

butlerx Mar 5, 2017

Author Contributor

they arnt templates they are technically md with json blobs at the beginning

});
});
liveShown = true;
}
}

function getHash() {
function getHash () {

This comment has been minimized.

Copy link
@SeanHealy33

SeanHealy33 Mar 5, 2017

Contributor

Is there a convention where you separate the fn name from param's like this?
Never seen this style before

This comment has been minimized.

Copy link
@butlerx

butlerx Mar 5, 2017

Author Contributor

http://eslint.org/docs/rules/space-before-function-paren
its eslints default can be disabled in .eslintrc but if we do id like it to be done on all the repos as they all use eslint now

This comment has been minimized.

Copy link
@SeanHealy33

SeanHealy33 Mar 5, 2017

Contributor

huh interesting 👍

$('.livestream').show(0, function() {
function showLive () {
if (!liveShown) {
$('.countdown').hide(400, () => {

This comment has been minimized.

Copy link
@SeanHealy33

SeanHealy33 Mar 5, 2017

Contributor

◀️ Functions ❤️

js/main.js Outdated
// right now.
// 3. Assign the value you chose back to the 'visited' in localStorage
localStorage['visited'] = localStorage['visited'] ? +localStorage['visited']+1 : 1;
/* This is a conditional (Ternary) Operator. It assigns a value based on some other value

This comment has been minimized.

Copy link
@SeanHealy33

SeanHealy33 Mar 5, 2017

Contributor

out of interest do we need a function explaining what a Ternary is?

Lot's of the comments here are just explaining what the code does. and should probably be removed since they are dead air

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Mar 5, 2017

Contributor

I added that when originally making the site so that people new to JS could follow what is going on.

This comment has been minimized.

Copy link
@butlerx

butlerx Mar 5, 2017

Author Contributor

leaveing this to #65

const schema = require(path.join(process.cwd(), 'schema.json'));
const config = require(path.join(process.cwd(), 'config.json'));

if (file !== undefined && file !== null) {

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Mar 5, 2017

Contributor

Would checking if(file) not work?

checkJson(i, json);
}
});
}

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Mar 5, 2017

Contributor

Can you flatten this block of code and divide it into different functions?

js/main.js Outdated
// right now.
// 3. Assign the value you chose back to the 'visited' in localStorage
localStorage['visited'] = localStorage['visited'] ? +localStorage['visited']+1 : 1;
/* This is a conditional (Ternary) Operator. It assigns a value based on some other value

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Mar 5, 2017

Contributor

I added that when originally making the site so that people new to JS could follow what is going on.

@@ -0,0 +1,115 @@
{{{

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Mar 5, 2017

Contributor

Template i presume. But if so i would rename the file from .json to .tmpl

@butlerx butlerx force-pushed the butlerx:enhancmentz/CFT branch 4 times, most recently from e1deda9 to 23be0f6 Mar 5, 2017

all issues solved

@butlerx butlerx force-pushed the butlerx:enhancmentz/CFT branch from 23be0f6 to 9a51ae1 Mar 5, 2017

@butlerx butlerx force-pushed the butlerx:enhancmentz/CFT branch from 9a51ae1 to 3908413 Mar 5, 2017

@SeanHealy33
Copy link
Contributor

left a comment

Not Too bad. I think you should try to do less with your prs since it feels like there is a yarn update a style/structure change and a few other things bundled into one pr 😄

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

This comment has been minimized.

Copy link
@SeanHealy33

SeanHealy33 Mar 5, 2017

Contributor

does err not throw by default from writeFile?

This comment has been minimized.

Copy link
@butlerx

butlerx Mar 5, 2017

Author Contributor

i dont actually know
/me checks
the docs say you should throw it

@@ -16,27 +16,22 @@ $(document).ready( function() {
});
});

// This is a conditional (Ternary) Operator. It assigns a value based on some other value

This comment has been minimized.

Copy link
@SeanHealy33

SeanHealy33 Mar 5, 2017

Contributor

:shade:

@butlerx

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2017

i agree with the yarn update i think i might draw up a doc/section in the readme about separating new dep and dep updates into separate prs

@butlerx butlerx merged commit c41a94d into redbrick:master Mar 5, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@butlerx butlerx deleted the butlerx:enhancmentz/CFT branch Mar 5, 2017

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.