-
Notifications
You must be signed in to change notification settings - Fork 3
✨ Add IDs to headings #24
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really promising! Great work so far :)
const getSlug = ( | ||
attributes: Record<string, any>, // eslint-disable-line @typescript-eslint/no-explicit-any | ||
children: RenderableTreeNode[], | ||
): string => { | ||
if (attributes.id && typeof attributes.id === "string") { | ||
return attributes.id; | ||
} | ||
return slugify(getTextContent(children), { | ||
lower: true, | ||
strict: true, | ||
}) as string; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my use case (and it's likely similar for others), it's important to control the slugification to get the desired output for URLs.
Looking at the source code for slugify
, the default locale replacements for the sv
locale would replace for example å
with aa
. However, that won't work with my requirements.
var locales = {
"sv":{"&":"och","Å":"AA","Ä":"AE","Ö":"OE","å":"aa","ä":"ae","ö":"oe"}
}
Proposed solutions
-
Allow users of the library to pass in the
Options
and/or callslugify.extend()
. However this adds the risk of breaking changes inmarkdoc-svelte
whenever theslugify
library is updated. Not very likely, but one should always be careful when exposing external types from third-party code. -
The other, and in my opinion much better option is to change the new option
headingIds
to have the typeboolean | (value: string) => string
, which allows passing in a custom slugger function. If the function is provided, we have a truthy value and thus know when to enable theheadingIds
slugger.
Option 2 allows us to:
- Enable and disable slugging by passing
headingIds: true | false
markdoc-svelte
could use the default slugger if the option is set totrue
.- And finally, we could use the custom slugger implemented by the user or
markdoc-svelte
I prefer the final option since that gives much more flexibility.
Curious to hear your thoughts about this! :)
NOTE: If we were to change the above, it might be suitable to implement as part of a follow-up branch, just to keep this one as clean as possible.
NOTE 2: If we go for option 2, and we have no other use for the slugger in markdoc-svelte
, then we might remove that dependency and let users provide a slugger function if they want slugged ids for headings. This simplified markdoc-svelte
.
There is also a third option - to use @sindresorhus/slugify
which exports an Option
type and has a good, flexible API. It has also been stable for the past years. If we were to use that library instead, I'd be happy to just pass in headingIds?: SlugifyOptions
and if the options are empty or non-existant, slugification is disabled. Otherwise enable it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea, and agree that option 2 is nice.
Originally added slugify only because it was quick and got the job done - did not think at all about the desired format of the slugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See if it works better for you now. I switched to slug
over slugify
as it seemed like the default was better (defaults to lowercase), which seemed to make it easier to set up if there aren't any requirements.
Closes #20
To do: