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

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

Merged
merged 4 commits into from
Nov 2, 2023

Conversation

Gutts-n
Copy link
Contributor

@Gutts-n Gutts-n commented Oct 17, 2023

Fixes this broken test: datopian/flowershow#576

Changes

  • Changed the regex to permit dashes

CC: @anuveyatsu @olayway @demenech

@changeset-bot
Copy link

changeset-bot bot commented Oct 17, 2023

🦋 Changeset detected

Latest commit: 1663b09

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
Copy link

vercel bot commented Oct 17, 2023

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) Visit Preview Nov 2, 2023 0:51am
portaljs-ckan ⬜️ Ignored (Inspect) Visit Preview Nov 2, 2023 0:51am
portaljs-ckan-ssg ⬜️ Ignored (Inspect) Visit Preview Nov 2, 2023 0:51am
portaljs-fivethirtyeight ⬜️ Ignored (Inspect) Visit Preview Nov 2, 2023 0:51am
portaljs-git-example ⬜️ Ignored (Inspect) Visit Preview Nov 2, 2023 0:51am
portaljs-learn ⬜️ Ignored (Inspect) Visit Preview Nov 2, 2023 0:51am
portaljs-openspending ⬜️ Ignored (Inspect) Visit Preview Nov 2, 2023 0:51am
portaljs-storybook ⬜️ Ignored (Inspect) Visit Preview Nov 2, 2023 0:51am
site-portaljs ⬜️ Ignored (Inspect) Visit Preview Nov 2, 2023 0:51am

Copy link
Member

@olayway olayway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gutts-n Thank you for submitting the PR. Good job! To make it even better I suggested a small change to the regex pattern.

Also, please remember to update the regex in this micromark html extension file.

Apart from that, could you please write unit tests for both the remarkWikiLink plugin and its micromark html extension? Please cover all the cases that seem to cause issues in Flowershow, like accented letters, hyphens, or parentheses.

And lastly, please add changeset file to this PR by running npx changeset in the root of the repo and following the prompts to describe the changes that this PR aims to apply.

packages/remark-wiki-link/src/lib/fromMarkdown.ts Outdated Show resolved Hide resolved
@olayway
Copy link
Member

olayway commented Oct 24, 2023

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

@Gutts-n Gutts-n requested a review from olayway October 26, 2023 03:00
Copy link
Member

@olayway olayway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look at my comments.
Also, we need to mirror these tests for the micromark extension, here: https://github.com/datopian/portaljs/blob/main/packages/remark-wiki-link/test/micromarkExtensionWikiLink.spec.ts

.changeset/old-planets-battle.md Outdated Show resolved Hide resolved
packages/remark-wiki-link/test/remarkWikiLink.spec.ts Outdated Show resolved Hide resolved
packages/remark-wiki-link/test/remarkWikiLink.spec.ts Outdated Show resolved Hide resolved
packages/remark-wiki-link/test/remarkWikiLink.spec.ts Outdated Show resolved Hide resolved
@Gutts-n
Copy link
Contributor Author

Gutts-n commented Oct 27, 2023

@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
Copy link
Member

olayway commented Oct 31, 2023

@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 🙂

@olayway olayway changed the title Fixing regex to not remove dashes "-" from markdown links Support wiki links with special characters and fix links to headings Oct 31, 2023
… of wiki links and adjusted the unit tests
@Gutts-n Gutts-n requested a review from olayway November 2, 2023 00:52
@Gutts-n
Copy link
Contributor Author

Gutts-n commented Nov 2, 2023

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
Copy link
Member

olayway commented Nov 2, 2023

@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 😃

@Gutts-n Gutts-n merged commit f17c2ed into main Nov 2, 2023
1 check passed
@github-actions github-actions bot mentioned this pull request Nov 2, 2023
@Gutts-n Gutts-n deleted the fix/broken-urls-with-dashes branch January 23, 2024 17:45
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.

2 participants