Conversation
…md as a .zip file
…sts + images linked in markdown
|
Thanks! This is fine @Blargian – we can work with this. It's probably my fault in the beginning (when I first set up the repo) for not being strict enough with code formatting. This review is on my todo list. I haven't forgotten 😄 |
jrobind
left a comment
jrobind
left a comment
There was a problem hiding this comment.
Apologies on the review delay.
This is an amazing PR – Great job! Thanks for taking the time to make such a substantial and helpful contribution. I've had a quick play around it seems to work great.
I've left a couple of minor comments. The main change is to revert the deletion of the .editorconfig file.
I think going forward, we can look to add in a build tool like webpack vs having vendor libraries copied in i.e. jzip & fileSaver. In the meantime it might be cleaner to move these into a vendor directory so it makes it easier for other contributors to differentiate between project source code and third-party source code.
Also, (beyond the scope of this PR for now) it would be nice to have some of the lesson export functions tested. Specifically, hasElementChild, mapTagToMd, and downloadFile just in case there are some edge cases that haven't been accounted for.
| //Retrieves the lesson object from localstorage given the lessonID | ||
| this.dataFromLocalStorage = function (lessonID) { | ||
| try { | ||
| return JSON.parse(localStorage.getItem("user")).lessons.filter( |
| markdown.push(mdOfElement + " \n"); | ||
| } | ||
| } | ||
| console.log(markdown); |
| return (string += line); | ||
| }, "") | ||
| ); | ||
| zip.generateAsync({ type: "blob" }).then(function (content) { |
There was a problem hiding this comment.
Do we need to safeguard against this async logic failing here? Could we wrap this in a try/catch or chain a .catch block?
| }; | ||
|
|
||
| const headingToMD = (className, textContent) => { | ||
| switch (className) { |
There was a problem hiding this comment.
Great use of a switch statement here!
| }; | ||
|
|
||
| let elements = parsedDocument.body.children; | ||
| for (let i = 0; i < elements.length; i++) { |
There was a problem hiding this comment.
nit: could we use a forEach here for consistency?
| @@ -1,12 +0,0 @@ | |||
| # EditorConfig helps developers define and maintain consistent | |||
There was a problem hiding this comment.
We don't want to remove the editorconfig. If you can add this back in that would be great :)
| }; | ||
|
|
||
| //Takes a lesson object and formats the data to be markdown | ||
| this.formatMarkdown = function (lessonObject) { |
There was a problem hiding this comment.
Destructuring can be a nice clean alternative for extracting values from object props. I.e.
function ({ title, content }) {
|
|
||
| //Stateful image counter function using a closure | ||
| //Increments by 1 each time it is called | ||
| const imageCounter = (function () { |
| }; | ||
| })(); | ||
|
|
||
| //Maps an htmltag to the correspinding md and pushes to markdown array |
There was a problem hiding this comment.
nit: small typo in the comment here
2 participants
Hi @jrobind, I tried a bunch of things but have ended up just removing the .editorconfig file. This last commit titled "removed .editorconfig" seems to have corrected the indentation issues and the diffs in the files I did change are showing correctly. Let me know if you're still having issues.