fix(project): image link, update CSS, add code highlighter #1259
Conversation
|
Hi @lasjorg, thanks for your hard work on this and for taking the time to write such helpful instructions. I took a look at this and noticed a few of things, one which may need some discussion. I'll leave them as review comments. |
| return `<a target="_blank" href="${href}">${text}` + '</a>'; | ||
| }; | ||
| // Use Prism for code highlighting | ||
| renderer.code = function (code) { | ||
| return `<pre><code class="language-javascript">${code}` + `</code></pre>\n`; |
There was a problem hiding this comment.
Very minor nitpick, but this could probably be simplified to:
| return `<pre><code class="language-javascript">${code}` + `</code></pre>\n`; | |
| return `<pre><code class="language-javascript">${code}</code></pre>\n`; |
| const renderer = new marked.Renderer(); | ||
| renderer.link = function(href, title, text) { | ||
| renderer.link = function (href, title, text) { | ||
| return `<a target="_blank" href="${href}">${text}` + '</a>'; |
There was a problem hiding this comment.
If you agree with the change below, would you mind also simplifying this line a bit?:
| return `<a target="_blank" href="${href}">${text}` + '</a>'; | |
| return `<a target="_blank" href="${href}">${text}</a>`; |
|
Also, since you're making a lot of improvements to this project, let's also update the link on line 141 to point to https://www.freecodecamp.org/. |
|
The last thing I noticed is a weird bug where Prism seems to add some extra whitespace to the To reproduce this, expand either the editor or the preview, then run the test. Then test 6 which reads "When my markdown previewer first loads, the default markdown in the #editor field should be rendered as HTML in the #preview element" will fail. This seems to be because Prism adds an extra space after rendering the code highlights. Our test suite gets the Sometime between those two points, if either the preview or editor pane are maximized, Prism renders It's a pretty minor thing, and I'm not sure how many people will actually run into this. And if you press the "Run Tests" button again, everything passes, at least until the editor or preview pane are resized again. It might be possible to strip off the extra whitespace introduced by Prism in test itself, but I can see that possibly introducing some edge cases. Then again, I'm thinking we might just leave this as-is since resizing the panes isn't a requirement. I don't think learners will run into this in their own projects if they don't add that feature. What do think @lasjorg? Is there a simple way to prevent Prism from adding that extra whitespace? Or should we just leave this the way it is for now? |
|
@scissorsneedfoodtoo Totally agree with everything. I was paying too much attention to the CSS, and at the last minute just threw in the code highlighter. Your thorough review is much appreciated. I think I came up with a solution that fixes the class name issue. I also just realized that the version of React and Marked we are using are pretty old. I updated them and everything still seems to work just fine. But I can revert them back if needed. Here is an updated Codepen as well |
|
@lasjorg, no worries. I like the addition of the code highlights, even if it's not a requirement. If anything it's another cool feature that learners can choose to implement themselves if they want. Also, I checked your updated pen and locally and no longer run into that issue where resizing the editor or preview pane causes the tests to fail. Thanks for diving back in and fixing that. Everything LGTM 👍 👍. Will just wait for another review before merging this and deploying your changes to CodePen with your helpful instructions above. |
|
Hey, sorry I missed this one - I'll try and give it a look in the next day or two. |
Pre-Submission Checklist
masterbranch of freeCodeCamp/testable-projects-fccfix/,feature/, ortranslate/(e.g.fix/signin-issue)Type of Change
Checklist:
Description
So I was going to just update the broken image link but then I noticed some issues with the responsiveness and started to change a few things. Before you know it it's a bit of a "refresh". I have updated the CSS and added a code highlighter as well just for good measure. The code highlighter implementation is likely not super well optimized (runs on componentDidUpdate) but I think it's fine.
Here is a Codepen fork for preview as well
https://codepen.io/lasjorg/pen/jOBGzgP
@scissorsneedfoodtoo When you update the Codepen (I assume it will be you) can you also set the HTML Preprocessor to none (click the cog on the left of the HTML box > set HTML Preprocessor from Haml to none). Not sure why it's there but it shouldn't be needed.
There is also a CSS file linked to in the Codepen Settings (Settings > CSS) which I don't believe is being used for anything.
https://cdnjs.cloudflare.com/ajax/libs/mocha/3.0.2/mocha.min.cssIt looks like some styles for mocha but the file isn't in any of the other projects and I can't see where its style would be used. It might just be some old CSS for an older version of the test script, not sure.