fix(v2): handle multiple asset links in one line properly

[lex111]

Motivation

Attempt to fix #3631

Note: I didn't quite understand why the parent node content should be changed now, if it's easier and safer to change the child node (which was done).

Previous tests are passed successfully + add new test for new use case.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Unit tests (I guess).

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

[docusaurus-bot]

Deploy preview for docusaurus-2 ready!

Built with commit 51fa150

https://deploy-preview-3653--docusaurus-2.netlify.app

[slorber]

Thanks, LGTM

@anshulrgoyal if there's a strong reason to modify the parent node it's time to tell us :)

[slorber]

Suggested change

	} >${linkText}</a>`;
	}>${linkText}</a>`;

Wonder if we can sately remove this space here? Seems not useful to keep it and unusual?
This would modify the snapshots but should be fine

[anshulrgoyal]

I think it is to support children in a tag

[slorber]

I think it is to support children in a tag

@anshulrgoyal can you give an example md snippet? not sure to understand what you mean by that but it may be worth adding an extra test case of this

[anshulrgoyal]

Something like this

[![image](url)](url)

[slorber]

oh, you mean an image link?
Didn't know it was possible to do that, but worth checking if this still works

[anshulrgoyal]

oh, you mean an image link?
Didn't know it was possible to do that, but worth checking if this still works

In MDX the file is converted to MDAST->HAST->MDXAST. So we can't just put link text between <a> and </a>

[lex111]

Indeed, image links do not work in the current implementation (because that use case is not covered by unit test), as well as rendering other (child) HTML tags inside the asset link (because type for all children is hardcoded as text).

In this PR I fixed these issues, see unit tests/snapshots for details.

I also did a small cleanup code (removed extra spaces in the output and unnecessary slug import , escaped title attribute).
So now I guess everything should be fine. @slorber please re-check this.

Note for future refactoring: instead of using string interpolation we need to come up with a clearer and better way to create a elements and dynamically add attributes to them.

Strange (?) thing: It seems that the handling of asset paths between transformLinks and transformImage is different (compare the body of toAssetLinkNode and processImageNode functions). For example, @site alias does not support in transformImage, that how it should be?

[anshulrgoyal]

Hi @lex111 If a child is JSX then how is it handled like:-

[<Button>Open</Button>](url)

[slorber]

Thanks @lex111 , LGTM!

Tried this in MDX playground, didn't even know it was possible 🤪

[<div style={{color: "red"}}>Open</div>](http://reactnative.dev/)

Does your code support it currently? Looks like it support MD in link but JSX?

Also what about adding a complex example of this dogfooding page?
https://v2.docusaurus.io/examples/markdownPageExample
This is a bit messy, we can refactor it a bit later but at least it ensures that it actually works in production :)

For example, @site alias does not support in transformImage, that how it should be?

Yes, this is a bit different, wanted to enable @site alias for both but didn't have time and had to release an urgent fix a few releases back 😅 if you can support @site alias in the other code as well that would be great :)

[lex111]

JSX inside asset Markdown link is already supported, although I don't think anyone will use it actively, but I updated the test to make sure that it really works.

[lex111]

Also what about adding a complex example of this dogfooding page?

Hmm, this is standard functionality that is assumed to work out of the box. I do not know which new use cases to add in the example demo page. So if you think you need to update this page, please feel free do it yourself.

[slorber]

thanks, let's merge this :)