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

URLs with spaces do not render a links in markdown cells #914

Closed
gnestor opened this issue Oct 5, 2016 · 16 comments
Closed

URLs with spaces do not render a links in markdown cells #914

gnestor opened this issue Oct 5, 2016 · 16 comments
Labels
bug: ui/ux jupyter spec outdated workflow: issue should stay closed
Milestone

Comments

@gnestor
Copy link
Contributor

gnestor commented Oct 5, 2016

Try the following in a markdown cell:

Working link: [Using Interact](Using%20Interact.ipynb)
Not working link: [Using Interact](Using Interact.ipynb)

image

@captainsafia
Copy link
Member

Hi Grant! Glad to see you're enjoying the nteract theme! 😄

We're using CommonMark which doesn't support spaces in links per its spec so I'm not sure this is something we can fix.

@jdetle
Copy link
Member

jdetle commented Oct 5, 2016

It's hacky and I know experienced developers hate regex but we could search/replace links before rendering them, no?

@rgbkrk
Copy link
Member

rgbkrk commented Oct 5, 2016

I love regex.

However, we have to take a step back here to understand and resolve the core problem while assisting the user as much as possible. Spaces have a specific meaning in Markdown URLs both in CommonMark and the original Markdown "spec":

To create an inline link, use a set of regular parentheses immediately after the link text’s closing square bracket. Inside the parentheses, put the URL where you want the link to point, along with an optional title for the link, surrounded in quotes. For example:

This is [an example](http://example.com/ "Title") inline link.

[This link](http://example.net/) has no title attribute.

A URL with spaces is not a valid URL. The URL with %20 is URL encoded, quite valid. There is an important reason to support the title attribute - for accessibility and screen readers. This becomes more important when doing links on images from Markdown.

@jdetle
Copy link
Member

jdetle commented Oct 5, 2016

Sorry my previous response came off as rude, didn't mean it to.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 5, 2016

In this case, I think the original jupyter notebooks (that I see above) need to be patched to match the markdown standards. If you copy a URL directly in Chrome that has spaces (assuming you're on the page, not that you've just typed it), Chrome copies the URL encoded form of the link.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 5, 2016

Sorry my previous response came off as rude, didn't mean it to.

Gosh no, I just didn't want you to think that using regexes themselves were bad. They're one of the fastest features to use in JS (perf wise). When used on the right problems they solve issues with alarming precision.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 5, 2016

We should probably move some of this discussion to https://github.com/jupyter/nbformat, since this affects the format.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 5, 2016

Heh, trying this over in http://spec.commonmark.org/dingus/ and it looks like title isn't supported - just rendering raw [stuff](url title)

@gnestor
Copy link
Contributor Author

gnestor commented Oct 5, 2016

@jdetle Commonmark returns a NodeWalker that would allow us to transform specific nodes before rendering. An example implementation for this specific use case:

const CommonMark = require('commonmark');
const parser = new CommonMark.Parser();
const renderer = new MarkdownRenderer();

const mdRender = input => {
  const parsed = parser.parse(input);
  const walker = parsed.walker();
  let event;
  while ((event = walker.next())) {
    const { node } = event;
    if (event.entering && node.type === 'link') {
      node.destination = encodeURI(node.destination);
    }
  }
  return renderer.render(parsed);
};

@rgbkrk I think this might work because Commonmark parses a link's destination and title, so transforming the destination shouldn't affect the title.

@gnestor
Copy link
Contributor Author

gnestor commented Oct 5, 2016

Well, one problem: Commonmark is typing these links with spaces as "text" and not "link", and it's splitting them up into multiple nodes vs. one text node.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 5, 2016

Awesome! Great to see you find out where to dig into the parser here.

Well, one problem: Commonmark is typing these links with spaces as "text" and not "link", and it's splitting them up into multiple nodes vs. one text node.

Oh yuck.

@rgbkrk rgbkrk added this to the Wishlist milestone Feb 7, 2017
@stale
Copy link

stale bot commented Apr 13, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale workflow: stale issue label Apr 13, 2018
@SetTrend
Copy link

I'm using "%20" URL encoding in my README.md file. Still GitHub renders the resulting hyperlinks incorrectly.

@stale stale bot removed the stale workflow: stale issue label Apr 13, 2018
@rgbkrk
Copy link
Member

rgbkrk commented Apr 13, 2018

That looks like some \ needs to become / for the directory paths. I'll send a PR your way.

@SetTrend
Copy link

Ah, I see ... Thanks a lot for having a look at my issue and helping me out on my mistake! 👍

@willingc
Copy link
Member

I'm going to close this since the link will work if the space is prefixed by \

@lock lock bot added the outdated workflow: issue should stay closed label Mar 19, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug: ui/ux jupyter spec outdated workflow: issue should stay closed
Projects
None yet
Development

No branches or pull requests

6 participants