Skip to content

Conversation

@hollisticated-horse
Copy link
Contributor

@hollisticated-horse hollisticated-horse commented Sep 6, 2025

This PR aims to fix #965.

The initial method would be to remove the SVG files or element, and replace it with either a #.
The presence of a hidden span with permalink could also be a pattern for recognition.

However, I am unable to create a test from a source file locally. I run it with the node command, but the content is not generated...

Comments on method or best practices are more then welcome.

Copy link
Contributor

@gijsk gijsk left a comment

Choose a reason for hiding this comment

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

Thanks for working on this and apologies for the slow response! I've been sidetracked by various other things including some time off.

The code change looks reasonable though I would probably not replace the SVG with anything at all?

However, I am unable to create a test from a source file locally. I run it with the node command, but the content is not generated...

Can you say more about this? What exactly are you running and what is the result?

Off-hand I would expect npm run generate-testcase <slug> to regenerate the testcase, and this WFM on my machine. Do you get an error instead?

I think updating the testcases is the only other thing that would stop this from being merged. 🙂

@hollisticated-horse
Copy link
Contributor Author

Thanks for working on this and apologies for the slow response! I've been sidetracked by various other things including some time off.

Yup, I understand completely. I would have eventually closed this PR in six months time 😁

The code change looks reasonable though I would probably not replace the SVG with anything at all?

My rational for replacing the svg with a "#" is to keep the anchor. I believe I saw some tests that expected anchors to stay and be displayed as #.

Can you say more about this? What exactly are you running and what is the result?

I'll get back to you on that, I seem to remember having managed to create the test case.

I think updating the testcases is the only other thing that would stop this from being merged. 🙂

That part scares me a bit. Can I modify expected content in those test cases that fail ? What are the guidelines for that ?

@gijsk
Copy link
Contributor

gijsk commented Sep 29, 2025

The code change looks reasonable though I would probably not replace the SVG with anything at all?

My rational for replacing the svg with a "#" is to keep the anchor. I believe I saw some tests that expected anchors to stay and be displayed as #.

Hm - can we only do this if there is nothing else in the link (ie there are no element children left)?

Can you say more about this? What exactly are you running and what is the result?

I'll get back to you on that, I seem to remember having managed to create the test case.

Thanks!

I think updating the testcases is the only other thing that would stop this from being merged. 🙂

That part scares me a bit. Can I modify expected content in those test cases that fail ? What are the guidelines for that ?

You can re-run npm run generate-testcase <dirname> (replacing <dirname>) to regenerate a specific testcase. That should make the relevant test pass. Then the github PR interface (or git diff locally) will make it easy to see what has changed, and accepting that will more or less be a judgment call from you and me. If this shows that, I don't know, we're accidentally replacing other things (emojis or bits of text) then you'd probably want to update the code so that testcases don't get undesirable changes, if that makes sense?

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.

Anchor permalinks display issues

2 participants