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

Fix ID shared by different pages being considered duplicates #311

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

romaricpascal
Copy link
Member

@romaricpascal romaricpascal commented Nov 21, 2022

Fix for #310 where pages sharing the same headings referenced in the navigation would end up with these headings treated as duplicates when their ID get generated for the navigation links. This would make the link point to the wrong ID and not fullfill their role of sending the user straight to the right section.

What’s changed

The commits should paint a step by step decription of the changes. As a broad summary, this PR changes when the UniqueIdentifierGenerator gets reset. It makes it happen before each Markdown render, rather than on Middleman's before hook (whose timing is still very unclear to me, but led to the original situation).

I did also try to tie the resets to a Rack middleware, to ensure they happen for every request made to Middleman (which surprisingly didn't match the before hook), but the issue remained.

The PR also cleans things up now the generator's lifecycle is bound to that of Redcarpet's renderer:

  • The Middleman extension is no longer necessary as the ID generation if tied to Redcarpet rather than Middleman.
  • The class no longer needs to be a Singleton (as it's all in the same place) or even reset (a new one can be instanciated for each document).

Each of these two bits is done in their own separate commit so could be easily dropped if we fear this may break things for people using the gem.

I've also added a few tests around the TechDocsHTMLRenderer to validate the heading generation.

Identifying a user need

This issue was discovered while adding JavaScript API documentation alongside the Sass one in the govuk-frontend docs. It may hit any documentation whose pages share the same heading structure (either written manually or automated).

Closes #310

Prevents IDs to be misinterpreted as duplicated across the rendering of different pages sharing headings
It is no longer needed for resetting the generator on the before lifecycle hook as it gets reset by the Markdown renderer
It no longer needs to be as its lifecycle is now fully tied to the Redcarpet renderer
@lfdebrux lfdebrux moved this from Prioritised backlog to Triage in Technical Documentation Template project board Dec 8, 2022
@lfdebrux lfdebrux moved this from Triage to Prioritised backlog in Technical Documentation Template project board Dec 8, 2022
@lfdebrux lfdebrux removed this from Prioritised backlog in Technical Documentation Template project board Dec 8, 2022
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.

Bug: Link in navigation point to the right ID when a heading is shared between two pages
1 participant