Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

underline links #244

Closed
wants to merge 1 commit into from
Closed

Conversation

jedahan
Copy link

@jedahan jedahan commented Dec 27, 2018

Added link underlining to unblock tldr-pages/tldr#2660

  • Code compiles correctly
  • Created tests, if possible
  • All tests passing (npm run test:all)
  • Extended the README / documentation, if necessary

@agnivade
Copy link
Member

agnivade commented Dec 28, 2018

Thanks ! Could you please add a test ? Shouldn't be very hard. Just take a look at test/parser.spec.js.

And no need to underline. Let us just keep the output simple to the eye.

And also update the commit message to -

display markdown links properly

We hook into the link event from the markdown parser and display the link.

@agnivade
Copy link
Member

ping @jedahan. Any chance you could take a look ? Thanks !

@vladimyr
Copy link
Collaborator

Homepages as defined per tldr-pages/tldr#2649 (comment) are commonmark autolinks. What @jedahan did is both:

  1. rendering links
  2. underlining them

Lets puts formatting aside for a moment. This means that tldr-node-client selectively renders only officially recognized parts of tldr pages while others are simply being ignored/hidden.
This is bad. Every piece of textual information should be preserved and rendered regardless if it is part of well defined structure or not. Looking inside lib/parser.js there is list of all possible markdown tokens:

const allElements = [
'blockquote', 'html', 'strong', 'em', 'br', 'del',
'heading', 'hr', 'image', 'link', 'list', 'listitem',
'paragraph', 'strikethrough', 'table', 'tablecell', 'tablerow'
];

followed by code that prevents their rendering:
allElements.forEach((e) => {
r[e] = () => { return ''; };
});

after which particular (officially recognized) sections are re-enabled, like strong (bold) for example:
r.strong = (text) => {
return chalk.bold(text);
};

I know it is highly unlikely that anyone will ever put ~deleted~ text inside tldr page but bear with me for a second. In that hypothetical case we would expect strikethrough method of our custom marked Renderer to get invoked and thus text will end up ignored; kinda ok but:

  1. There is no such thing as Renderer.prototype.strikethrough, it will actually call .del instead.
  2. We have just hidden part of textual information from users eyes without them knowing ⚠️

I hear you saying that won't ever happen except it might happen right away if there is any link inside of any currently available tldr pages. I'm not talking about homepages explicitly, there might be links used for demonstration purposes inside example description section and they will simply get swallowed because they are blacklisted by default. ⚠️

This brings me to the point of graceful degradation. Wonderful example of such approach is text only renderer shipped as part of marked itself (exposed as marked.TextRenderer):
https://github.com/markedjs/marked/blob/88a856137646c183196ce96632c290d78ab27ee0/lib/marked.js#L1079-L1098
It follows simple rule: if it has textual representation you'll get one, otherwise token gets ignored.
Instead of being binary exclusive in terms of what tldr-node-client renders lets bring in some shades of gray in between using following fall through steps:

  1. if it is officially recognized token render it using custom rendering fn (like list items)
  2. if it has textual representation render that instead
  3. if we end up here that means we have no other options but to ignore it

In practical terms that means subclassing marked.TextRenderer instead of doing full blacklist and re-applying our custom rules on top of that. That would prevent us of maintaining list of possible tokens (we failed to do, no strikethrough folks 🤦‍♂️) and override only what's needed. In the same time we get zero cost & worries graceful degradation.
In our particular case links would work with zero changes needed because marked parses autolinks in following manner:

<https://example.com> → { "href": "https://example.com", "text": "https://example.com" }

and piping that through:
https://github.com/markedjs/marked/blob/88a856137646c183196ce96632c290d78ab27ee0/lib/marked.js#L1091-L1094
you would get following rendered:

https://example.com

Not bad I guess? 😉

Long story short lets fix things instead of patching them :bowtie:
@jedahan @agnivade Any thoughts? Are you ok with my proposal?


Going back to question of link formatting. I don't have opinion on underline vs. plain styling but I do have objections regarding proposed homepage formatting as showcased here:
https://github.com/tldr-pages/tldr/raw/5e846e2/pages/common/7z.md

> Homepage: <https://www.7-zip.org/>.

Once this goes through marked we'll end up with following output:

Homepage: https://www.7-zip.org/.

This trailing dot is really a nice way to play let's guess what would terminal emulator do game 🙅‍♂️ In some terminal emulators it might be wrongly treated as part of url (especially if it ends with .html suffix) while in others it will get omitted from clickable section. I suspect this is one of reasons why people often put urls in angle brackets knowing that it's guaranteed they can not be part of an url so what's inside simply gets clickable.
/cc @sbrl

That being said my vote goes to render it in brackets please. 🙏

There is also this awesome new emerging standard: https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda
already supported by iTerm 🎉 and simple enough by means of terminal-link so we could progressively enhance it if possible.

/shameless plug ahead

Click to reveal 😁

Example implementation can be found here:
https://github.com/vladimyr/tldr-lite/blob/40c7fac300b23ec5da227d03fa59ad4b5c51f2e1/lib/parser.js#L115-L119
In order to testdrive it you can do following:
curl -s https://github.com/tldr-pages/tldr/raw/5e846e2/pages/common/7z.md | npx tldr-lite

@jedahan
Copy link
Author

jedahan commented Jan 14, 2019

Honestly I don't even use the node client, and I did think it weird that the client was hiding information.
I just wanted to do the minimum patch that would move along having links in tldr-pages.
I'm all for changing things to be progressively enhanced from regular markdown rendering.

@agnivade
Copy link
Member

Subclassing and customising the list of tokens sounds good to me.

Regarding brackets vs angle brackets, please comment on the original issue so that other folks get to know.

Thanks for looking into this.

@agnivade agnivade mentioned this pull request Jan 30, 2019
@agnivade
Copy link
Member

Closed in favor of #245

@agnivade agnivade closed this Jan 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants