Skip to content
This repository has been archived by the owner on May 30, 2023. It is now read-only.

[UX] Markdown Previewer Optional Challenge (#7) #305

Closed
vipatron opened this issue Apr 21, 2018 · 16 comments · Fixed by #527
Closed

[UX] Markdown Previewer Optional Challenge (#7) #305

vipatron opened this issue Apr 21, 2018 · 16 comments · Fixed by #527
Assignees

Comments

@vipatron
Copy link

Issue Description

Unit Tests/#Test text (reproduced on the [Beta challenge waypoint here] states that:

  1. OPTIONAL BONUS (you do not need to make this test pass): When I click a link rendered by my markdown previewer, the link is opened up in a new tab (HINT: read the Marked.js docs for this one!)
  2. OPTIONAL BONUS (you do not need to make this test pass): My markdown previewer interprets carriage returns and renders them as
    (line break) elements

Problem: The marked.js documentation is actually necessary for Test #8 (line break behavior). The link to the relevant page here is easily found from their github page. However, the Optional test #7 isn't an option I was able to find, nor is it strictly speaking necessary.

It is possible that I missed it, but I dove deep: I read the documentation at a superficial level, and at a deep one, even reading the link sections of the gfm and commonmark specifications, the source code of marked.js, and browsing closed issues on their github, which is where I found this comment created just YESTERDAY, Apr 20 2018 in which the author shows how to not lose the input sanitization of marked, but rather, to add on top of it. I tried to implement it within my react class, and failed, since ES6 classes don't seem to support class properties, just member functions. At some point, I realized that I could probably just use string.replace() to modify the output, and that worked. Without rebuilding the whole library, I don't know how I could make marked() itself return links whose target is another tab. Again, while it's possible that I'm just missing something, and I found the experience to be enlightening, since it forced me to explore so much out of the bounds of the curriculum, I think those with a lower frustration tolerance than I have would get discouraged. Thoughts?

Your Code / Link to Your Pen

Not relevant, but here's my project pen

@vipatron
Copy link
Author

@tbushman thx for replying, but are you talking about this issue regarding the regex for the blockquote test?

@vipatron
Copy link
Author

@tbushman Yes, it was a doozy to research. There are two ways to make the output from marked() do it:

  1. Edit the source code to create a new option (waaaay too much work)
  2. Replace the renderer's { link } function with a wrapper version of the original link function that just does a regex replace on the output before returning it. As I mentioned in my OP, this solution was offered as the latest comment on a closed thread about this issue on 4/20

@tchaffee
Copy link
Contributor

@vipatron There is a comment from 2015 explaining a workaround. markedjs/marked#655 (comment)

But I don't think a comment in a workaround is what we should be asking campers to research for a bonus feature. I recommend we remove this bonus feature. @QuincyLarson what do you think?

@no-stack-dub-sack
Copy link
Member

no-stack-dub-sack commented Jun 10, 2018

@tchaffee @tbushman @vipatron It looks like the Marked docs have changed considerably since I wrote these tests... back then, a quick look at the docs would have done it, I remember it being a pretty easy bonus at that point, meant to encourage folks to dig in to docs a bit (@vipatron seems like it worked! but sorry it resulted in so much frustration).

The 2nd solution you mentioned above is the one I was looking for (also present in the example pen, for anyone else coming upon this issue, or below):

// ALLOWS LINE BREAKS WITH RETURN BUTTON
marked.setOptions({
  breaks: true,
});

// INSERTS target="_blank" INTO HREF TAGS (required for codepen links)
const renderer = new marked.Renderer();
renderer.link = function (href, title, text) {
  return `<a target="_blank" href="${href}">${text}` + '</a>';
}

However, I agree that without adequate documentation at present (I no longer see anything about custom renderers in the docs), this might as well be removed.

p.s. Kudos to you @vipatron for your thorough research of this issue! That's a great sign that this career path is right for you :-)

@QuincyLarson
Copy link
Contributor

@no-stack-dub-sack Awesome - thanks for giving us some more perspective on this.

@vipatron would you be interested in opening a PR to fix this?

@QuincyLarson
Copy link
Contributor

@no-stack-dub-sack Thanks for giving us some additional background on this.

@vipatron would you be interested in opening a PR to fix this?

@tchaffee
Copy link
Contributor

tchaffee commented Jul 5, 2018

@vipatron Any interest in helping us out with a PR? If you don't have time that's ok and we can assign it to someone else. Thanks.

@QuincyLarson
Copy link
Contributor

@vipatron Are you interested in helping us fix this?

@ValeraS
Copy link
Contributor

ValeraS commented Aug 16, 2018

The documentation contains information about the custom renderer for a heading token https://marked.js.org/#/USING_PRO.md

@tchaffee
Copy link
Contributor

Thanks @ValeraS for the link. I think with just that page it's too hard for Campers to figure out and would take a long time (as it did with @vipatron ). I already have a PR submitted to remove this bonus requirement.

@ValeraS
Copy link
Contributor

ValeraS commented Aug 16, 2018

It's an optional bonus, why remove it?
After reading the doc., you can find the solution using google: "marked renderer.link".

@tchaffee
Copy link
Contributor

@ValeraS we already discussed it (above) and decided it takes too much research. Google results are not the same for everyone, they are actually personalized, and we cannot count on that search result always being there. Back when I did this it took me only a few minutes to find it in the official docs and then implement it. @vipatron went through WAY more than that. That's not what we want as a Camper experience. Thanks for your input though, it's appreciated.

@ValeraS
Copy link
Contributor

ValeraS commented Aug 16, 2018

@tchaffee What should be the campers' experience? Do they have to do only what the documentation says directly?

@vipatron did the challenge and learned more than others who will not.

@tchaffee
Copy link
Contributor

We don't want Campers struggling for hours trying to find the solution for something that isn't clearly documented. It's frustrating. That is not how the bonus requirement was originally designed. The original bonus requirement was supported by clear documentation. It isn't anymore and that makes the requirement way harder. Sorry, but it's just no longer the right fit, we already decided what to do as a team which included the person who wrote the challenge, and I've gone forward with the fix. This isn't worth spending time on. The project without that frustrating requirement works well and there is still a different bonus requirement that gets Campers looking at the docs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@tchaffee @QuincyLarson @ValeraS @no-stack-dub-sack @vipatron and others