Fix ID generation #240
Fix ID generation #240
Conversation
To match Ruby conventions as we're going to have more than one ruby file in our project
romaricpascal
force-pushed
the
fix-id-generation
branch
from
00162e3 to
fae4097
Compare
✅ Deploy Preview for govuk-frontend-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
Nice fix @romaricpascal, is it something we can open as a PR on https://github.com/alphagov/tech-docs-gem? |
I think on the |
There was a problem hiding this comment.
Happy to approve this as a temporary fix (otherwise it'll bite us again)
Some questions, but not blockers:
-
Shall we wait until it's fixed upstream?
alphagov/tech-docs-gem? -
What is the patch commit going to solve?
The commit message doesn't say 😊
For 1) I'm not sure how long (or complex) an upstream fix is. I'd wait if it's only a quick one, otherwise it's complex and has breaking changes for other teams maybe not
Add a new `preprocess` hook to the Redcarpet renderer to reset the ID generator. This ensures headings that may appear on multiple pages are not treated as duplicate when rendering one of these pages. See: alphagov/tech-docs-gem#310
romaricpascal
force-pushed
the
fix-id-generation
branch
from
fae4097 to
78f40d4
Compare
|
Updated the commit message for the patch to quickly describe why and point to the issue in the tech docs repo. Regarding the first question, this PR is there to have things fixed in our docs waiting for upstream to be fixed, so it wouldn't make sense to wait for it to be fixed before merging 😄 |
Temporary fix waiting for a more long term solution to this issue to be implemented in the tech-docs-gem repository: alphagov/tech-docs-gem#310
The broken computation of IDs seems to be due to the the ID generator being reset a bit too loosely. This PR ensures it get reset for the rendering of each page, which makes sure it only considers the IDs on the page itself.
For that it hooks onto the
preprocessstep of Redcarpet's Markdown rendering to reset the ID generator.The
TechDocsHTMLRendereris used in two different places, though (for the general rendering and the Sass API ref). This means we cannot extend it and replace with our own renderer.Instead this PR uses Ruby's ability to add methods to existing classes. To make sure this is done cleanly, the
preprocessmethod is defined in a separate module and thenincluded in the class separately. This:superifTechDocsHTMLRendererstarts implementing apreprocessitself (though hopefully it'll be to land the fix, so that'll mean we can get rid of this patch)