Add external link icon (#136)

[kukimik]

This adds an icon to external links. I've checked it on Firefox 94.0 and Chromium 95.0.4638.69. Screenshots attached.

Mostly it looks ok, but look at the image (FAIR Pledge) at the bottom of the first screenshot: the icon is displayed in the next line.

So this is not quite ready to get merged. Do you have any ideas how to handle the image case? I'm not even sure about the expected behavior: should we also have an icon if the link content is just an image? What if it has some text but ends with an image?

I'd also like to know your opinion whether adding the emanote-external class to external links is the right way to go.

[srid]

About the FAIR pledge, yes that's a problem; not sure what the solution here is.

About the emanote-external class, that looks fine ... but we should allow the user to disable this styling if they wish. I've been meaning to split pandoc.tpl into separate files (another good-first-issue for Haskell if anyone wants to open a PR), so once that happens - uses can customize their link styles by creating a pandoc/link.tpl file.

Feedback on design: the external link icon should be of grey color (for de-emphasiz) and not of blue ($theme).

---

Is there anyway we can generalize this to also implement (clickable) header anchors? eg: https://markkarpov.com/tutorial/megaparsec.html#controlling-backtracking-with-try

It would need backend support, but assuming it exists ...

[kukimik]

Feedback on design: the external link icon should be of grey color (for de-emphasiz) and not of blue ($theme).

Ok. If you want it grey, then we do not need all this mask trickery and we can just use an image with a fixed color.

Is there anyway we can generalize this to also implement (clickable) header anchors?

I don't think CSS can be used for this. Without backend support one could add some JS that adds an anchor after each header once the page is loaded, but I wouldn't call this a pretty solution.

However, if the following TODO in pandoc.tpl is done, then I think the anchor can just be inserted as part of the template?

<!-- TODO: Expand the above kind of overriding (full DOM control) to other AST nodes (below) -->

I've been meaning to split pandoc.tpl into separate files (another good-first-issue for Haskell if anyone wants to open a PR), so once that happens - uses can customize their link styles by creating a pandoc/link.tpl file.

I think that's a good idea. I know some Haskell, so I could help with this, but at the moment I don't have much free time. Where (in which modules) should this be implemented? I'm not familiar with the structure of emanote yet.

[kukimik]

Feedback on design: the external link icon should be of grey color (for de-emphasiz) and not of blue ($theme).

How about this?

About the FAIR pledge, yes that's a problem; not sure what the solution here is.

Fixed this with JS that disables the icon if the <a> element contains an image. This may need some tweaking to handle other situations (e.g. <svg> instead of <img>). See the script at the bottom of base.tpl.

About the emanote-external class, that looks fine ... but we should allow the user to disable this styling if they wish. I've been meaning to split pandoc.tpl into separate files (another good-first-issue for Haskell if anyone wants to open a PR), so once that happens - uses can customize their link styles by creating a pandoc/link.tpl file.

What do you think about also having a separate CSS file (or a set of files?) instead of putting the styles inside base.tpl (or inlining them in pandoc.tpl)?

[srid]

Sorry I let this slip my radar.

[srid]

Feedback on design: the external link icon should be of grey color (for de-emphasiz) and not of blue ($theme).

How about this?

About the FAIR pledge, yes that's a problem; not sure what the solution here is.

Fixed this with JS that disables the icon if the <a> element contains an image. This may need some tweaking to handle other situations (e.g. <svg> instead of <img>). See the script at the bottom of base.tpl.

I think I'd prefer the JS do the opposite. ie., start with no icon next to links, and have the JS add the icon to all text links (links with text, not image). It is safer approach, as we can't handle all edge cases.

About the emanote-external class, that looks fine ... but we should allow the user to disable this styling if they wish. I've been meaning to split pandoc.tpl into separate files (another good-first-issue for Haskell if anyone wants to open a PR), so once that happens - uses can customize their link styles by creating a pandoc/link.tpl file.

What do you think about also having a separate CSS file (or a set of files?) instead of putting the styles inside base.tpl (or inlining them in pandoc.tpl)?

Inline styles is fine for now. If they become huge, we can move it latter in a different PR.

---

@kukimik The screenshot you pasted looks good. Once the JS thing is addressed, we can merge it (after I test it locally).

[kukimik]

I think I'd prefer the JS do the opposite. ie., start with no icon next to links, and have the JS add the icon to all text links (links with text, not image). It is safer approach, as we can't handle all edge cases.

On the other hand, with the current approach you can easily turn off the external link icon manually in a particular case by adding a class to a single link. With JS activating the icon this is, of course, also possible, but I think it gets a little bit more complicated.

[srid]

The generated site should work on browsers with JavaScript disabled.

That's another -- and arguably the most important -- reason for the JS injecting icons after page load.

[kukimik]

@srid Please have a look at this. The links containing an image in the link text are now recognized before HTML is generated, so no JavaScript is needed. I'm not sure if Heist.Extra.Splices.Pandoc.Ctx is the best place to do it; it seemed to be the easiest one. If the user wants to manually disable the icon for some external link, they just need to add the emanote-external-no-icon class to the link. To completely disable the feature one has to override base.tpl.

Perhaps the code in Ctx.hs could be made shorter (this is somewhat FizzBuzz-like), and probably the feature should be documented, but I want to know your opinion before fixing the details.

Sorry for taking so long. Family reasons.

[kukimik]

What do you think: should the icons appear in the backlinks section?

I also see I need to fix the colors to make them consistent with other icons around.

[srid]

I'll take a look later this week. Been busy.

[srid]

@kukimik Just took a look at this.

• For backlinks, that can be done as part of Tasklist checkbox does not render in backlinks panel #187 (not necessarily in this PR)
• Your containsImage logic doesn't work for wikilinks: [![[disaster-girl.jpg]]](https://google.com) (I see the external icon), though it does work for regular links [![](disaster-girl.jpg)](https://google.com). You can change the logic so that icon is disabled when link text contains any wiki-links (embed or not).
• We should add tests for this logic (checks for plain text, images and wiki-links including embed ones); see bin/test. As well as, actually use these image links (both wikilink and normal) in the demo docs.

I can refactor the code latter.

[srid]

Behaviorally, the icon looks good to me (and selecting text doesn't select it anymore). Maybe it should be made smaller. What do you mean by "fix the colors"?

[kukimik]

For backlinks, that can be done as part of #187 (not necessarily in this PR)

I believe it's sufficient to add the emanote-external classes in context.tpl. Looks like it works.

Your containsImage logic doesn't work for wikilinks: ![[disaster-girl.jpg]] (I see the external icon), though it does work for regular links . You can change the logic so that icon is disabled when link text contains any wiki-links (embed or not).

I disabled the icon if the link contains another link. I believe this should detect wikilinks. Am I missing something?

We should add tests for this logic (checks for plain text, images and wiki-links including embed ones); see bin/test. As well as, actually use these image links (both wikilink and normal) in the demo docs.

TBD

and selecting text doesn't select it anymore

AFAIR this was an issue in #190, not here.

Maybe it should be made smaller.

Made it a bit smaller.

What do you mean by "fix the colors"?

I thought that the color for inactive icon should be lighter (like the color of the "Edit this page" icon), but I checked it and it doesn't look good - the icons are hardly visible. The current colors are ok.

[srid]

Okay, behaviourally things look good to me 👍🏿

Why do we need NoIcon differentiation at all? Can't we get rid of it?

[kukimik]

Why do we need NoIcon differentiation at all? Can't we get rid of it?

I'm not sure if I understand the question correctly.

External links without an icon still need to behave like other external links (e.g. class="hover:underline" target="_blank" rel="noopener" in pandoc.tpl). So we have external links with an icon and external links without an icon.

I see the following options to implement this:

• For external links inject or omit the emanote-external class directly in the code, without using templates. (Or maybe we can have two classes: emanote-external-icon and emanote-external-no-icon, and inject the right one. Or use some other variant of this approach. The main point is that we do it in .hs, not in .tpl)
  • This is against the tendency to keep styling in the templates. But as long as the classes are described in the docs, I'm ok with it.
• Keep it all in the templates. That's what I did.
  • I believe that this way the emanote-external* classes may seem less "magical" to the end user, and may be understood by just looking at the templates, without reading the docs and without reading Haskell.
  • It also allows the user to easily apply some other tailwind styling when the icon can't be used. But probably this extra flexibility is not needed.
  • On the other hand, it clutters the templates.

There are some mixed solutions, but I don't find them very clean.

If you find the first approach better, then I may rewrite it. It should be easy.

[srid]

@kukimik Basically, I meant removing the NoIcon thing entirely. I just tried it out and it works. See 9dee614

Do you see any issue with this change? I cannot think why users would want to customize the "no icon" case. Can you point out a real-world example?

[kukimik]

Do you see any issue with this change?

The external links containing an image (or a wikilink) lose the target='_blank' and rel='noopener' attributes. These attributes are set by:

<External class="hover:underline emanote-external" target="_blank" rel="noopener" />

That's what I meant here:

External links without an icon still need to behave like other external links (e.g. class="hover:underline" target="_blank" rel="noopener" in pandoc.tpl). So we have external links with an icon and external links without an icon.

On the other hand this made me think some more about my proposed solution and now I think that the following structure would be better:

<External [no extra class here]>
  <WithIcon class="emanote-external-icon">
</External>

This, together with an extra emanote-external-no-icon class and a minor change in Ctx.hs, would allow the user to easily force showing/hiding the icon for particular links. (Previously they were only easy to hide, but I think forcibly showing them is a more common case, e.g. I'd like to be able to force an external icon for <a href='...'>A link text containing some <img src='fancy_symbol.png'> in the form of an inline image</a>.

---

I cannot think why users would want to customize the "no icon" case. Can you point out a real-world example?

I don't have such an example. That's why I said "probably this extra flexibility is not needed."

[srid]

Let's take a step back and flesh out the requirements first.

Question 1: What are the specific Markdown snippets on which we do not want to add the external icon? So far, one has been identified:

1. An image wrapped in a link (see the disaster girl image here). (Note: this doesn't include your "A link text containing some image" case).

Is there anything else? I'm not sure if we should bother with wiki-links since that is an absurd case (is there a legitimate reason for them?).

Question 2: Why do we need more than one template class var, External, here? This is my original question: why the separate WithIcon/WithNoIcon thing? Let's see some specific examples in Markdown. It may even be better to use them in ./docs/demo.

[srid]

An image wrapped in a link

Actually, this is only valid if the image appears as a block element (as with the disaster girl, due to being the only element of a <p>). Which makes the requirement even more annoying to property define.

EDIT: But maybe the inline icon is undesirable even in inline images?

[srid]

I'm mostly envisioning a situation where the user doesn't have to override their .tpl - tweaking WithIcon/NoIcon just to workaround the external link icon behavior. It should do the "right" thing as default.

[srid]

Yea, this is really frustrating to think about without having specific and concrete examples. I'd suggest adding all those examples (including your "A link text containing some image ..." case) to ./docs/demo in some form first. It could just be a sub-heading on the 'Extended Markdown' page.

Then defining the requirements becomes straightforward, and it is easy to talk about this from a common ground.

[kukimik]

This is going to be a heuristic. A better or worse one, but still. I don't believe we can cover all the usecases. I'm trying to illustrate this in the custom_styling.md section. I propose to show the icon by default if the link contains some text, and hide it otherwise.
This can be easily overridden by supplying an appropriate class.

I also propose an implementation that I find satisfying. I think it's pleasantly symmetric and readable at the cost of slight redundance.

No tests yet. I've not looked into this.

[kukimik]

I've noticed that I duplicate the emanote-external-icon/emanote-external-no-icon classes if they are set by hand. Are you ok with changing <> to Data.List.union in this line? https://github.com/EmaApps/emanote/blob/d2ae1013094537f76692edc910bf36b2fb543107/src/Heist/Extra/Splices/Pandoc/Attr.hs#L21

[srid]

@kukimik I'm happy with this PR. If you could add tests for determineLinkType (which contains the heuristic), I can look into merging it after a cleanup. See the various *Spec.hs files for comparison.

[kukimik]

Rebased to master to run treefmt using new Ormolu.

[srid]

I think this is ready to be merged! Just one thing I don't understand in base.tpl.

[kukimik]

@srid Thanks for the final corrections. And for your guidance throughout the process. And for your patience - the PR was opened over a year ago! Glad to see it merged.