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.

Sign up for GitHub

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

Jump to bottom

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

Open
romaricpascal opened this issue Nov 14, 2022 · 1 comment · May be fixed by #311
Open

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

romaricpascal opened this issue Nov 14, 2022 · 1 comment · May be fixed by #311
Projects
Technical Documentation Template proj...

Comments

@romaricpascal
Copy link
Member

What should change

The computation of unique IDs for the links in the navigation gives the wrong results when:

  • two pages share a common heading
  • you're on one of these two pages
  • the navigation has a link to the other page pointing to the shared heading

The link points to an ID composed of the heading and the previous heading (for ex. /javascript-api-reference/#javascript-api-reference-button), as if the target was on the current page. It should instead be only the heading (in the example /javascript-api-reference/#button) as on the other page, the ID is unique.

To reproduce:

  • checkout this commit of govuk-frontend-docs (dd1d088), - npm install,
  • bundle install
  • bundle exec middleman server
  • Navigate to "Sass API Reference"
  • Expand the menu item for "JavaScript API Reference"
  • The "Button" link will point to #javascript-api-reference-button, while when browsing the other pages, it points to #button

User need

When publishing the latest release of the design system, we had our JavaScript API Reference and Sass API Reference both list the Button component, leading to the issue described. A situation that may happen further as both docs get expanded and list the options for our components. Other documentations with repetitive structure/content across pages may run into this issue as well.
@romaricpascal
Copy link
Member Author

I think the issue is not so much with how the headings are computed by the GovukTechDocs::UniqueIdentifierGenerator as with where the cache of existing headings is reset.

At the moment it is done by GovukTechDocs::UniqueIdentifierExtension. Haven't quite figured out when exactly it triggers. The docs mention "before Rack requests" plural. I may be misunderstanding, but I think it's not "once before each request to generate the content of a page". It does trigger multiple times when loading a page though1.

That said, I think it the reset of the generator should be tied more closely to the markdown generation, using Redcarpet's preprocess hook instead of being tied to the overall Middleman lifecycle. This would limit the risks of leakage.

Bluntly, this could be done via:

    # lib/govuk_tech_docs/tech_docs_html_renderer.rb
    def preprocess(document)
      UniqueIdentifierGenerator.instance.reset
      return document
    end

However, given that would put the ID generation all in one place, this means we could stop GovukTechDocs::UniqueIdentifierGenerator from being a singleton and just instanciate a new one each time we pre-process. Or at least store the instance in the renderer rather than having it as a global singleton.

Footnotes

  1. Investigated through editing the files in ~/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/govuk_tech_docs-3.1.0 and logging when things got reset with pp calls ↩

@romaricpascal romaricpascal mentioned this issue Nov 16, 2022
Merged
romaricpascal added a commit to alphagov/govuk-frontend-docs that referenced this issue Nov 17, 2022
@romaricpascal
Patch GovukTechDocs::TechDocsHTMLRenderer
78f40d4 
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 romaricpascal linked a pull request Nov 21, 2022 that will close this issue
Open
@lfdebrux lfdebrux moved this from Triage to 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
Assignees
No one assigned
Labels
None yet
Projects
Technical Documentation Template proj...
Prioritised backlog
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix ID shared by different pages being considered duplicates romaricpascal/tech-docs-gem
1 participant
@romaricpascal