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

Enable nested headings using the isNested option #227

Closed
wants to merge 5 commits into from

Conversation

dhruvkb
Copy link
Contributor

@dhruvkb dhruvkb commented Feb 1, 2023

Description 📖

This pull request adds a new isNested boolean option the @islands/headings module. It can be used as follows:

import { defineConfig } from 'iles'

import headings from '@islands/headings'

export default defineConfig({
  modules: [
    headings({ isNested: true }) // `true` for nested headings, `false` otherwise
  ],
})

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 the headings object becomes a tree instead of a list. Now the headings only contains the list of top-level headings, with all others available inside their children field recursively.

Screenshots 📷

See the following Debug output from a dummy post in my îles blog project.

{
  "layout": "post",
  "frontmatter": {
    "path": "/blog/posts/my_title",
    "title": "My Title",
    "date": "2023-01-07T00:00:00.000Z",
    "isFeatured": true
  },
  "meta": {
    "filename": "src/pages/blog/posts/my_title.mdx",
    "lastUpdated": "2023-01-31T20:05:42.835Z",
    "href": "/blog/posts/my_title",
    "title": "Duis ultrices augues",
    "headings": [
      {
        "level": 1,
        "title": "Duis ultrices augues",
        "slug": "duis-ultrices-augues",
        "children": [
          {
            "level": 2,
            "title": "Morbi eu iaculis",
            "slug": "morbi-eu-iaculis",
            "children": [
              {
                "level": 3,
                "title": "Sed dapibus",
                "slug": "sed-dapibus",
                "children": [
                  {
                    "level": 4,
                    "title": "Fusce a mollis mauris",
                    "slug": "fusce-a-mollis-mauris",
                    "children": [],
                    "indices": [
                      1,
                      1,
                      1,
                      1
                    ]
                  }
                ],
                "indices": [
                  1,
                  1,
                  1
                ]
              }
            ],
            "indices": [
              1,
              1
            ]
          }
        ],
        "indices": [
          1
        ]
      },
      {
        "level": 1,
        "title": "Phasellus dui dui",
        "slug": "phasellus-dui-dui",
        "children": [
          {
            "level": 6,
            "title": "Mauris at augue vel ante",
            "slug": "mauris-at-augue-vel-ante",
            "children": [],
            "indices": [
              2,
              1
            ]
          }
        ],
        "indices": [
          2
        ]
      }
    ],
    "excerpt": "This is my post."
  },
  "props": {}
}

@nx-cloud
Copy link

nx-cloud bot commented Feb 1, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 50c8fbe. 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


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@ouuan
Copy link
Contributor

ouuan commented Feb 1, 2023

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.

@ElMassimo
Copy link
Owner

ElMassimo commented Feb 1, 2023

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.

@dhruvkb
Copy link
Contributor Author

dhruvkb commented Feb 1, 2023

@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.

@ouuan
Copy link
Contributor

ouuan commented Feb 1, 2023

Maybe we can simply provide a function that converts the flat format to the tree format?

@dhruvkb
Copy link
Contributor Author

dhruvkb commented Feb 1, 2023

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.

@ouuan
Copy link
Contributor

ouuan commented Feb 1, 2023

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.

@ouuan
Copy link
Contributor

ouuan commented Feb 1, 2023

What about adding these two features:

  1. A function to transfer from array to tree (perhaps without indices information), for users that simply want a tree structure of the headings. I guess this is enough for common use cases.
  2. An option, which is a function, whose input is the headings array and an array of the hast nodes of the headings, and the output is an array of hast nodes. Then the corresponding indices in the AST (children of the root node) are replaced by the outputs. The default function is to set the id and add an anchor tag inside the <h*> and return the <h*> itself. If you need tree-structured information, you can generate it by yourself or use the provided flat-to-tree function to help you, but the output needs to be a flat array (just do a DFS of the tree).

@dhruvkb
Copy link
Contributor Author

dhruvkb commented Feb 1, 2023

@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.

@dhruvkb dhruvkb closed this May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants