🔗 Fix URLs in table of contents directive #2140
Conversation
🦋 Changeset detectedLatest commit: 3db2a50 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
brianhawthorne
changed the title
packages/myst-transforms/src/toc.ts
Outdated
| return item; | ||
| } | ||
|
|
||
| function listItemChildFromPage(page: ProjectPage, projectSlug?: string): Text | Link { |
There was a problem hiding this comment.
Confusing function name they used here, took me a while to figure out what's going on 🙃
packages/myst-transforms/src/toc.ts
Outdated
| const page = pages[0]; | ||
| const child = listItemChildFromPage(page, projectSlug); | ||
| const item = makeListItem(child); | ||
| if (pages[1] && pages[1].level > page.level) { |
There was a problem hiding this comment.
Looks like nesting is happening here; may be worth pointing out in a comment.
packages/myst-transforms/src/toc.ts
Outdated
| return item; | ||
| } | ||
|
|
||
| function listItemChildFromHeading(heading: Heading): Text | Link { |
There was a problem hiding this comment.
Do we have a way of documenting these? I think even a simple one sentence description would be quite helpful for future readers!
There was a problem hiding this comment.
I added a small bit of documentation. The mutually recursive function pairs here are turning me a little bit cross-eyed honestly. Since I don't fully understand the logic, I'm afraid if I add too much by way of explaining, it might end up misleading.
I did note that the two mutually recursive function pairs (listFromPages/listItemFromPages and listFromHeadings/listItemFromHeadings) are largely identical modulo the details of the base-case transformation and the choice of .level vs .depth as the property that controls the recursion. If they were to settle on either level or depth for both kinds of TOC trees, then the four could be combined into a single mutually recursive pair parametrized by a node transformer function. I wasn't sure if the choice to use two different property names to represent what looks like the same thing in these two cases was a design decision or just incidental... maybe an opportunity to refactor, but not something I'd be inclined to take on just yet 😁 .
There was a problem hiding this comment.
Probably something @fwkoch would know!
|
Note some linting errors too; perhaps try fixing with |
There was a problem hiding this comment.
Thanks, @brianhawthorne! This looks good to me. We'll have to get @fwkoch or someone else on the team to take a second look, since I don't know this code well enough to merge.
rowanc1
changed the title
|
Will get this in shortly - @brianhawthorne thanks for the contribution!! |
tocdirective #2119