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

Add autoID for definition terms #13403

Closed
bep opened this issue Feb 15, 2025 · 11 comments · Fixed by #13404
Closed

Add autoID for definition terms #13403

bep opened this issue Feb 15, 2025 · 11 comments · Fixed by #13404
Assignees
Milestone

Comments

@bep
Copy link
Member

bep commented Feb 15, 2025

We have a pretty good search setup on gohugo.io, but it's very annoying that we cannot link directly to the Definition terms.

These term listings are very convenient for documenting properties etc.

So, I will add autoID support for the terms

So,

Base Name
: Base name of the file.

Foo@Bar
: The foo bar.

Becomes

 <dl>
  <dt id="base-name">Base Name</dt>
  <dd>Base name of the file.</dd>
  <dt id="foobar">Foo@Bar</dt>
  <dd>The foo bar.</dd>
</dl>

Markdown parser config changes:

  • autoHeadingID (existing)
  • autoDefinitionTermID (new)
  • autoHeadingIDType renamed to autoIDType (it's not possible/practical to have 2 setting for this)

IDs will be added to Page.Fragments.Identifiers.

I will make sure that autoHeadingIDType will still work after this.

Note that the will share the same "auto ID context" as headings, so there will be no duplicate IDs.

@bep bep self-assigned this Feb 15, 2025
@bep bep removed the NeedsTriage label Feb 15, 2025
@bep bep added this to the v0.144.0 milestone Feb 15, 2025
bep added a commit to gohugoio/hugoDocs that referenced this issue Feb 15, 2025
This reverts commit c08c8e1.

This will eventually fix itself with gohugoio/hugo#13403
@jmooring
Copy link
Member

Questions:

  • Will these become part of the page .Fragments?
  • If a heading and a dt have the same text, will they have duplicate id's?
  • If the dt is a link, can we use the same id generation as GitHub (see #13401)? e.g.,
foo [something](/a/b/) bar
: A foo bar.

In the above, it would be nice if we matched GitHub's convention --> #foo-something-bar

@jmooring
Copy link
Member

Related: #11566

@bep
Copy link
Member Author

bep commented Feb 15, 2025

Will these become part of the page .Fragments?

No. Or at least not now (it would certainly would require some more thinking -- and having "auto ids" will solve the problem I'm seeing now, so I will focus on that for now).

If a heading and a dt have the same text, will they have duplicate id's?

No. they share the same "auto ID context", so they will get a counter appended on conflicts.

If the dt is a link, can we use the same id generation as GitHub

I will have it in mind.

@jmooring
Copy link
Member

No. Or at least not now (it would certainly would require some more thinking -- and having "auto ids" will solve the problem I'm seeing now, so I will focus on that for now).

I understand. The primary reason that I asked about this was the link checking that we do on the docs site, which currently verifies fragments on internal links. So we'll lose that if we convert the h6 hacks to dt's.

@bep
Copy link
Member Author

bep commented Feb 15, 2025

@jmooring -- OK, thinking a little, this is the Fragments struct:

type Fragments struct {
	// Headings holds the top level headings.
	Headings Headings

	// Identifiers holds all the identifiers in the ToC as a sorted slice.
	// Note that collections.SortedStringSlice has both a Contains and Count method
	// that can be used to identify missing and duplicate IDs.
	Identifiers collections.SortedStringSlice

	// HeadingsMap holds all the headings in the ToC as a map.
	// Note that with duplicate IDs, the last one will win.
	HeadingsMap map[string]*Heading
}

I assume

  • You use the Identifiers to check that a fragment exists?
  • I can certainly make sure that the description terms IDs is also put into this slice (but not in HeadingsMap)
  • Would that work?

@jmooring
Copy link
Member

Yes, that would work. Beginning of identifier validation in link render hook...

  {{- with $u.Fragment }}
    {{- if $p.Fragments.Identifiers.Contains . }}
      {{- if gt ($p.Fragments.Identifiers.Count .) 1 }}

@bep
Copy link
Member Author

bep commented Feb 15, 2025

Yes, that would work. Beginning of identifier validation in link render hook...

OK, then that is the new plan.

@jmooring
Copy link
Member

Thank you. The link checking has been very helpful, if not indispensable . And I really hate my h6 hack.

@bep
Copy link
Member Author

bep commented Feb 15, 2025

Couldn't the check be simplified to:

  {{- with $u.Fragment }}
      {{- if gt ($p.Fragments.Identifiers.Count .) 1 }}

???

@jmooring
Copy link
Member

jmooring commented Feb 15, 2025

There are 2 checks:

  1. Look for duplicate IDs on the page (e.g., user sets one of them using a Markdown attribute, the other id is auto-generated)
  2. Make sure that the ID exists on the page (the actual link check)
code
  {{- /* Validate. */}}
  {{- with $u.Fragment }}
    {{- if $p.Fragments.Identifiers.Contains . }}
      {{- if gt ($p.Fragments.Identifiers.Count .) 1 }}
        {{- $msg := printf "The %q render hook detected duplicate heading IDs %q in %s" $renderHookName . $contentPath }}
        {{- if eq $errorLevel "warning" }}
          {{- warnf $msg }}
        {{- else if eq $errorLevel "error" }}
          {{- errorf $msg }}
        {{- end }}
      {{- end }}
    {{- else }}
      {{- /* Determine target path for warning and error message. */}}
      {{- $targetPath := "" }}
      {{- with $p.File }}
        {{- $targetPath = .Path }}
      {{- else }}
        {{- $targetPath = .Path }}
      {{- end }}
      {{- /* Set common message. */}}
      {{- $msg := printf "The %q render hook was unable to find heading ID %q in %s. See %s" $renderHookName . $targetPath $contentPath }}
      {{- if eq $targetPath $contentPath }}
        {{- $msg = printf "The %q render hook was unable to find heading ID %q in %s" $renderHookName . $targetPath }}
      {{- end }}
      {{- /* Throw warning or error. */}}
      {{- if eq $errorLevel "warning" }}
        {{- warnf $msg }}
      {{- else if eq $errorLevel "error" }}
        {{- errorf $msg }}
      {{- end }}
    {{- end }}
  {{- end }}

@bep bep added Enhancement and removed Proposal labels Feb 15, 2025
@bep bep pinned this issue Feb 15, 2025
@bep bep unpinned this issue Feb 15, 2025
@bep bep pinned this issue Feb 15, 2025
bep added a commit to bep/hugo that referenced this issue Feb 16, 2025
bep added a commit to bep/hugo that referenced this issue Feb 16, 2025
bep added a commit to bep/hugo that referenced this issue Feb 16, 2025
bep added a commit to bep/hugo that referenced this issue Feb 16, 2025
bep added a commit to bep/hugo that referenced this issue Feb 16, 2025
bep added a commit to bep/hugo that referenced this issue Feb 16, 2025
bep added a commit to bep/hugo that referenced this issue Feb 16, 2025
bep added a commit to bep/hugo that referenced this issue Feb 16, 2025
@bep bep changed the title Add autoID for decription terms Add autoID for definition terms Feb 16, 2025
bep added a commit to bep/hugo that referenced this issue Feb 16, 2025
bep added a commit to bep/hugo that referenced this issue Feb 16, 2025
bep added a commit to bep/hugo that referenced this issue Feb 16, 2025
Fixes gohugoio#13403
See gohugoio#11566

Co-authored-by: Joe Mooring <joe@mooring.com>
bep added a commit to bep/hugo that referenced this issue Feb 16, 2025
Fixes gohugoio#13403
See gohugoio#11566

Co-authored-by: Joe Mooring <joe@mooring.com>
@bep bep closed this as completed in 157d370 Feb 16, 2025
@bep bep unpinned this issue Feb 17, 2025
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants