datopian / datahub

🌀 Rapidly build rich data portals using a modern frontend framework
https://datahub.io/opensource
MIT License
2.17k stars 322 forks source link

Support wiki links with special characters and fix links to headings #1040

Closed Gutts-n closed 7 months ago

Gutts-n commented 7 months ago

Fixes this broken test: https://github.com/datopian/flowershow/pull/576

Changes

CC: @anuveyatsu @olayway @demenech

changeset-bot[bot] commented 7 months ago

🦋 Changeset detected

Latest commit: 1663b09a860726ed39bebba34fe634e48a46afcb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | -------------------------- | ----- | | @portaljs/remark-wiki-link | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel[bot] commented 7 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

9 Ignored Deployments | Name | Status | Preview | Comments | Updated (UTC) | | :--- | :----- | :------ | :------- | :------ | | **portaljs-alan-turing** | ⬜️ Ignored ([Inspect](https://vercel.com/datopian1/portaljs-alan-turing/5HLCbtDFhHgehoStEAPqMZDng2pc)) | [Visit Preview](https://portaljs-alan-turing-git-fix-broken-urls-with-dashes-datopian1.vercel.app) | | Nov 2, 2023 0:51am | | **portaljs-ckan** | ⬜️ Ignored ([Inspect](https://vercel.com/datopian1/portaljs-ckan/GW8dcvDuT7LF97aAHoyAyzzp9T4S)) | [Visit Preview](https://portaljs-ckan-git-fix-broken-urls-with-dashes-datopian1.vercel.app) | | Nov 2, 2023 0:51am | | **portaljs-ckan-ssg** | ⬜️ Ignored ([Inspect](https://vercel.com/datopian1/portaljs-ckan-ssg/Aq77Vqe6yhG4vkZLnKfyHaH9QfP3)) | [Visit Preview](https://portaljs-ckan-ssg-git-fix-broken-urls-with-dashes-datopian1.vercel.app) | | Nov 2, 2023 0:51am | | **portaljs-fivethirtyeight** | ⬜️ Ignored ([Inspect](https://vercel.com/datopian1/portaljs-fivethirtyeight/F6D3UDuKXL6tCDJkxbSauJwqmvY3)) | [Visit Preview](https://portaljs-fivethirtyeight-git-fix-broken-urls-w-a5a345-datopian1.vercel.app) | | Nov 2, 2023 0:51am | | **portaljs-git-example** | ⬜️ Ignored ([Inspect](https://vercel.com/datopian1/portaljs-git-example/NZZsjyHrXRMHdKhAnCKofVwE2R54)) | [Visit Preview](https://portaljs-git-example-git-fix-broken-urls-with-dashes-datopian1.vercel.app) | | Nov 2, 2023 0:51am | | **portaljs-learn** | ⬜️ Ignored ([Inspect](https://vercel.com/datopian1/portaljs-learn/3TKSQdHwFFvs9TNMKTvm8XwLgZYc)) | [Visit Preview](https://portaljs-learn-git-fix-broken-urls-with-dashes-datopian1.vercel.app) | | Nov 2, 2023 0:51am | | **portaljs-openspending** | ⬜️ Ignored ([Inspect](https://vercel.com/datopian1/portaljs-openspending/BhMPtSh3e629x8yytfzFRHpxUwJQ)) | [Visit Preview](https://portaljs-openspending-git-fix-broken-urls-with-dashes-datopian1.vercel.app) | | Nov 2, 2023 0:51am | | **portaljs-storybook** | ⬜️ Ignored ([Inspect](https://vercel.com/datopian1/portaljs-storybook/5JvzSHSyrrTEjq2PiMWZeu53Spcp)) | [Visit Preview](https://portaljs-storybook-git-fix-broken-urls-with-dashes-datopian1.vercel.app) | | Nov 2, 2023 0:51am | | **site-portaljs** | ⬜️ Ignored ([Inspect](https://vercel.com/datopian1/site-portaljs/vLuTBaat2CQxSLDBHgDvuLUC3q8x)) | [Visit Preview](https://site-portaljs-git-fix-broken-urls-with-dashes-datopian1.vercel.app) | | Nov 2, 2023 0:51am |
olayway commented 7 months ago

@Gutts-n Do you need any help? :)

Gutts-n commented 7 months ago

@olayway in tests here after the # in links is switching spaces with dashes, is this behavior expected? In obsidian we can link a file + header with spaces inside of another file. image image

Is happening just in the first space of the linked file image With a dash on the first position, it'll change the second space image

olayway commented 7 months ago

@Gutts-n No it definitely shouldn't replace only the first space 😅 Good catch!

And the whole spaces->dash + lowercasing of headings was just a temporary solution that was supposed to produce hrefs matching id's generated by rehype-slug, which is another package we use in Flowershow to add ids to headings (so that they can be linked to). However, rehype-slug's generation of id from the content of the heading is not this simplistic. They use github-slugger under the hood. Please look at examples of input and output headings of rehype-slug here: https://github.com/rehypejs/rehype-slug#use. As you can see, there's more than just replacing spaces with dashes and lowercasing. It would be great if you'd like to work on that too sometime 🙂 How I would do it is I'd make the remark-wiki-link plugin generate headings using github-slugger as well by default, and I'd allow users to pass their own custom transformer.

For now, please just replace this:

    const headingId = heading.replace(/\s+/, "-").toLowerCase();

With this:

    const headingId = heading.replace(/\s+/g, "-").toLowerCase();

in both html.ts and fromMarkdown.ts, and adjust/add relevant test cases.

Then we can merge 🙂

Gutts-n commented 7 months ago

I finished here @olayway, just one thing that I noted is that the hChildren property of the tags is being generated this way, is it the expected scenario?

image

olayway commented 7 months ago

@Gutts-n yes, this is correct. This is basically making sure that the text node inside of the anchor tag will have this value. And here it should be exactly the same as the text in the wikilink as there was no alias set. Here is the part of code where it is being set:

https://github.com/datopian/portaljs/blob/f1d7e68077ca3655befa83788017415ade6b589b/packages/remark-wiki-link/src/lib/fromMarkdown.ts#L135

And here is some docs explaining what are these hProperties or hChildren used for:

https://github.com/syntax-tree/mdast-util-to-hast#fields-on-nodes

That being said... Well done!!! Great job! 🎉 Let's merge 😃