Skip to content

Conversation

@alfonsobries
Copy link
Member

@alfonsobries alfonsobries commented Oct 7, 2021

Summary

Dependant of: https://github.com/ArkEcosystem/ark.dev/pull/191

The old version of the thephpleague/commonmark library used to add a name attribute to the header permalinks (the links inside the #) since that name matches the link the internal links worked properly. (See anchor links on https://ark.dev/docs/glossary/glossary)

The current version, however, doesn't add that attribute meaning the links doest work. (See anchor links on ark.dev PR glossary section)

That name is necessary to make the anchor link works since the id have a user-content- prefix that doesn't match the link

This PR replaces the renderer for a new one that is exactly the same but adds the name attribute.

Note:
The current alternative on this PR replicates the exact old behavior, a simple alternative is to remove the prefix on the IDs, which also works (at least on the glossary section), the problem is that I cannot say for sure that all the links point to the id and also the id can potentially interfere with other ids in the site, for example, if we have a header with User as title and a form with an input#user both will have the same ID, I don't think is very likely to happen but not impossible so I think it worth a discussion.

Permalinks Before:

<a id="user-content-2fa" ... >#</a> 2FA

Permalinks After:

<a id="user-content-2fa"  name="2fa" ... >#</a> 2FA

Checklist

  • I checked my UI changes against the design and there are no notable differences
  • I checked my UI changes for any responsiveness issues
  • I checked my (code) changes for obvious issues, debug statements and commented code
  • I provided a screenshot of my changes to the component (if applicable)
  • I regenerated the icons.html file and checked if my newly added icon is shown correctly (if necessary)
  • I added an explanation on how to use the component to the readme (if necessary)
  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

@alfonsobries alfonsobries changed the title remove id_prefix on heading_permalink fix: add name attribute to header permalink Oct 7, 2021
@faustbrian faustbrian merged commit 63900c4 into main Oct 8, 2021
@faustbrian faustbrian deleted the fix/heading-permalin-ido branch October 8, 2021 00:36
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.

3 participants