Generate Epic README.md#199
Generate Epic README.md#199Supermn54 wants to merge 2 commits intoolistic:masterfrom Supermn54:epicReadme
Conversation
olistic
left a comment
olistic
left a comment
There was a problem hiding this comment.
@Supermn54 Thanks for your PR! It has some errors/opportunities for improvements, please check my comments. Also, I pushed a commit I'd done a while ago that will be very useful for you, feel free to cherry-pick it or base your branch in my topic branch instead of master.
| getFloorMap, | ||
| getFloorMapKey, | ||
| profile: this.profile, | ||
| level: this.level, |
There was a problem hiding this comment.
You just need to change the level key to an array of levels, not the entire data object. The other three keys remain the same no matter the level.
| const options = { filename: README_TEMPLATE_FILE_PATH }; | ||
| const renderedReadme = ejs.render(template, data, options); | ||
| fs.writeFileSync(this.profile.getReadmeFilePath(), renderedReadme); | ||
| if (this.profile.epic) { |
There was a problem hiding this comment.
You should use this.profile.isEpic() here.
| const renderedReadme = ejs.render(template, data, options); | ||
| fs.writeFileSync(this.profile.getReadmeFilePath(), renderedReadme); | ||
| if (this.profile.epic) { | ||
| for (let i = 1; i < 10; i += 1) { |
There was a problem hiding this comment.
Check my comments here explaining what we should use for the loop.
| levels.push(data); | ||
| const renderedReadme = ejs.render(template, levels, options); | ||
| fs.writeFileSync(this.profile.getReadmeFilePath(), renderedReadme); | ||
| } |
There was a problem hiding this comment.
The data object and the last two lines of this method can be moved outside the if/else blocks. The overall structure of the generateReadmeFile method should look something like this:
const template = fs.readFileSync(README_TEMPLATE_FILE_PATH, 'utf8');
const data = {
getFloorMap,
getFloorMapKey,
profile: this.profile,
};
if (this.profile.isEpic()) {
data.levels = ... // <-- Here you assemble the array of levels (all the levels)
} else {
data.levels = ... // <-- Here you assemble the array of levels (only one level in this case)
}
const options = { filename: README_TEMPLATE_FILE_PATH };
const renderedReadme = ejs.render(template, data, options);
fs.writeFileSync(this.profile.getReadmeFilePath(), renderedReadme);|
|
||
| ## Level <%- level.number %> | ||
| <% levels.forEach(level => { -%> | ||
| <%- include('levels', { level }); %> |
There was a problem hiding this comment.
You need to use the exact include line below instead of 'levels' (which you haven't defined yet?). Also, the header should be rendered inside the loop.
| getFloorMap, | ||
| getFloorMapKey, | ||
| profile: this.profile, | ||
| level: i, |
There was a problem hiding this comment.
You're just passing the level number here, where we need the level config object instead. Check what we pass inside this.level, that should be what we pass for each level here. Also, check my general comment. The commit I talk about will make this much easier and cleaner.
| */ | ||
| prepareEpicMode() { | ||
| this.profile.enableEpicMode(); | ||
| this.generateProfileFiles(); |
2 participants
This is to address issue #159. Ran tests and passed all except for 1 which failed because ProfileGenerator.js test file was expecting an object, not an array. To create the array, I ended up using a fixed array size for now. I noticed the number of levels for both the beginner and intermediate towers were the same.
I welcome any feedback and the opportunity to make changes. My javascript/EJS template skills are really rusty so I appreciate the opportunity to work on this.