Closed dhruvkb closed 1 year ago
CI is running/has finished running commands for commit 50c8fbe5b89aaeae42cb189081e175976c715224. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.
๐ See all runs for this branch
Sent with ๐ from NxCloud.
I suggest changing the option name to format: 'flat' | 'tree'
.
slug
and initData
(HeadingOptions
) are also supposed to be configurable by the user. But I'm not sure how the configs could be passed in the old code, and the new code seems like the correct way to do it ๐ค Maybe I'm wrong, but anyhow they shouldn't be treated differently.
Something that comes to mind is that we lose type safety since there's no straightforward way for @islands/headings
to augment PageMeta
with a different Heading
type based on whether format
is flat
or tree
.
Perhaps it would make sense to publish separately as @islands/headings-tree
(that is equivalent to isNested: true
or format: 'tree'
), so that it can augment PageMeta
differently.
This module would share and reuse code with @islands/headings
, but for backwards compatibility would stay unchanged.
@ElMassimo the type Heading
is essentially the same with and without nesting, except with children: Heading[]
being empty if nesting is disabled. So I don't see much value in separating the project into two and maintaining two packages. That's my view as a contributor but I respect your opinion as the maintainer.
@ouuan I don't know how slug
and initData
are configured currently and will need to look into that. Maybe we can start by exporting those functions so that they can be modified and extended in userland.
Maybe we can simply provide a function that converts the flat format to the tree format?
That was my first approach and it would be enough if the only requirement was to tree the flat list of headings but my usecase needed me to make modifications to the HTML node like adding data-attrs for the indices.
That was my first approach and it would be enough if the only requirement was to tree the flat list of headings but my usecase needed me to make modifications to the HTML node like adding data-attrs for the indices.
I didn't notice this. It sounds like a very specific use case and not all users would want it. Maybe we can have something more generic, like customizing the HTML elements by a function based on some context. Actually, I also want this feature for my use case.
What about adding these two features:
@ouuan what you are describing in point 2 could potentially be accomplished with some tweaking of the existing slug
and initData
fields of HeadingOptions
. I'll explore that.
If you prefer, feel free to close this PR because this is very different from your idea of the ideal implementation.
Description ๐
This pull request adds a new
isNested
boolean option the@islands/headings
module. It can be used as follows:Background ๐
See #226
The Fix ๐จ
By changing the
rehypePlugin
to automatically insert the current heading as a child of the last known heading of higher level, the structure of theheadings
object becomes a tree instead of a list. Now theheadings
only contains the list of top-level headings, with all others available inside theirchildren
field recursively.Screenshots ๐ท
See the following Debug output from a dummy post in my รฎles blog project.