Removes trailing dash from generated slugs in markdown #3044
Conversation
🦋 Changeset detectedLatest commit: 6f53106 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
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 |
|
How does this play with other plugins? For example, a plugin that adds a table of content may generate slugs with trailing dash, making it unusable (or not, I'm just asking) |
|
@JuanM04 This change will apply to all the plugins that generate slugs from headings. |
There was a problem hiding this comment.
Let's just remove trailing dashes only when we are generating them. Otherwise, we should let the plugins do their own. Something like this:
if (!node.properties) node.properties = {};
if (!node.properties.id) {
let slug = slugger.slug(text);
if (slug.endsWith('-')) slug = slug.slice(0, -1);
node.properties.id = slug;
}
headers.push({ depth, slug: node.properties.id, text });
.changeset/cold-bears-sneeze.md
Outdated
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@astrojs/markdown-remark': patch | |||
There was a problem hiding this comment.
the fact that this could break some existing URL hashes means that we'll need to:
- think this through a bit more than normal: we want to make this change once and only once, if we can help it.
- time it with other breaking changes, if we have any to go out together
- message it as a potentially breaking change in the changelog
There was a problem hiding this comment.
Hmm. You are right Fred. But then should it be a minor or major change? Will there be a v0.27?
|
@JuanM04 I don't see any difference between the code you provided and the current code. Can you explain what that changes? |
|
@RafidMuhymin yeah, sorry, I had a typo. That code only trims the dash if the slug was generated by us instead of a custom plugin |
|
Yeah, you are right @JuanM04 ! Made some changes to the PR! |
There was a problem hiding this comment.
I still want to see this merged, but we may want to hold off until v1.0.0-rc.1, where we can bundle a bunch of final breaking changes together one last time.
@RafidMuhymin I don't want you to have to keep this PR up to date forever, so if you can change this PR to merge into v1-rc branch instead of main, I can help you merge and help keep it up to date, as we move closer to our RC!
|
@RafidMuhymin are you planning on updating this? Thanks! |
|
Yup, I'd originally asked RafidMuhymin to hold off on merging this since it's a tiny but breaking change. Now that we're getting ready to switch |
There was a problem hiding this comment.
Looks good! @FredKSchott, the changeset needs to be major, minor or patch?
|
|
|
Merging this PR. |
This reverts commit 8530cce.
This reverts commit 8530cce.
* fixed header slugs in markdown if ends with a dash * added changeset * removes trailing dash only if slug was created * updated test * updated change level from patch to minor
Changes
If the slugs generated from headers in markdown end with a trailing dash, the dash will be removed. This change will improve slug generation in the following cases.
### `<BackgroundImage />`Currently, the generated slug will be
backgroundimage-. But with this change, it will be justbackgroundimage.Testing
The change was tested locally.
Docs