Skip to content

Add data-dictionary attribute to glossary-first handlebar - #2442

Draft
Kuuuube wants to merge 2 commits into
yomidevs:masterfrom
Kuuuube:glossary-first-data
Draft

Add data-dictionary attribute to glossary-first handlebar#2442
Kuuuube wants to merge 2 commits into
yomidevs:masterfrom
Kuuuube:glossary-first-data

Conversation

@Kuuuube

@Kuuuube Kuuuube commented Jun 7, 2026

Copy link
Copy Markdown
Member

Using a div here instead of an li since this handlebar hasnt used that previously. A little bit wonky since the single-glossary handlebars do use an li even though it's also only ever one glossary. Maybe that should be changed but outside the scope of this PR.

@Kuuuube
Kuuuube requested a review from a team as a code owner June 7, 2026 04:02
@Kuuuube Kuuuube added kind/enhancement The issue or PR is a new feature or request area/anki The issue or PR is related to Anki integration labels Jun 7, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9357171356

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +156 to +157
<div data-dictionary="{{dictionary}}">
{{~> glossary-single . brief=../brief noDictionaryTag=../noDictionaryTag ~}}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Render term glossary-first from the definition context

For non-grouped term definitions this block is still in the root render context, so dictionary and . are not the term definition; the previous template passed definition explicitly. In that scenario {glossary-first} now emits an empty data-dictionary and an empty glossary/tag block (the updated test expectations include data-dictionary="" and <ul></ul>), which breaks ordinary term cards using this marker. Pass definition to glossary-single and read definition.dictionary here, and apply the same correction in the v77 upgrade template.

Useful? React with 👍 / 👎.

@7hosi

7hosi commented Jun 7, 2026

Copy link
Copy Markdown

Lapis relies on the <ul><li></li></ul> structure, and other note types might as well. I think the change to a <div> for when there is one glossary would be better done to all handlebars later (if at all, since it would break people's cards).

@Kuuuube
Kuuuube marked this pull request as draft June 16, 2026 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/anki The issue or PR is related to Anki integration kind/enhancement The issue or PR is a new feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants