-
Notifications
You must be signed in to change notification settings - Fork 8
add testing of all json file, linting of main js and gulp as test #30
Conversation
9ae1a21
to
4f55e7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 💃
4f55e7a
to
b84f676
Compare
JSON.parse(fs.readFileSync(filename, "utf8")); | ||
console.log("Look good"); | ||
JSON.parse(fs.readFileSync(filename, 'utf8')); | ||
console.log(filename + ' look good'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/look/looks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@butlerx ping on this before I approve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved
if(file !== undefined && file !== null) { | ||
checkJson(file); | ||
} else { | ||
for(var i of ['events.json', '2015/events.json', '2016/events.json']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't hardcode the years in. Also, replace var
with let
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since they arnt modified in the loop it should prob be const
also suggestion where too put the array or form it im not a big fan of adding for json files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we decide to migrate historic data it would grow a bit. We can move the 2015 and 2016 into a new directory years
, and iterate over every year we find there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep sounds like a plan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ive opened issue #31 to do with the migration part as an idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, cool
var ul = day.day+'-events'; | ||
var de = $('<li><div id="'+day.day+'" class="collapsible-header container day"><h5 class="day__title" data-position="right"><span class="day__title__bold">' + day.day + '</span> - ' + day.description + '</h5></div><div class="collapsible-body"><ul id="'+ul+'"class="collapsible sub-collapsible" data-collepsible="accordion"></div></ul>').appendTo(ce); | ||
var ul = day.day + '-events'; | ||
var de = $('<li><div id="' + day.day + '" class="collapsible-header container day"><h5 class="day__title" data-position="right"><span class="day__title__bold">' + day.day + '</span> - ' + day.description + '</h5></div><div class="collapsible-body"><ul id="'+ul+'"class="collapsible sub-collapsible" data-collepsible="accordion"></div></ul>'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realise this line was long before, but maybe split it to make it less terrible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cant breaks minifaction
b84f676
to
7491d2e
Compare
84c9a19
to
72553e7
Compare
for #26