-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
add new "substitute" behavior #21
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #21 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 261 289 +28
=========================================
+ Hits 261 289 +28
☔ View full report in Codecov by Sentry. |
Hey! Thank you for working on this. Appreciate all the thinking you did. There’s one gotcha with their new approach: links in headings. ## a [b](c) d <h2 tabindex="-1" id="user-content-a-b-d" dir="auto">
<a class="heading-link" href="#a-b-d">a </a>
<a href="/wooorm/private/blob/main/c">b</a>
d
<svg ...></svg>
</h2> ## [a](b) c <h2 tabindex="-1" id="user-content-a-c" dir="auto">
<a class="heading-link" href="#a-c"></a>
<a href="/wooorm/private/blob/main/b">a</a>
c
<svg ...></svg>
</h2> Honestly I think it’s pretty wild that they completely forgot about links... |
OK, so I think two things are missing:
|
What do you think of this to allow content with wrap? @@ -96,7 +96,7 @@ export default function rehypeAutolinkHeadings(options) {
const settings = options || emptyOptions
let props = settings.properties
const behavior = settings.behavior || 'prepend'
- const content = settings.content || contentDefaults
+ const content = settings.content
const group = settings.group
const is = convertElement(settings.test)
@@ -133,7 +133,7 @@ export default function rehypeAutolinkHeadings(options) {
/** @type {import('unist-util-visit').Visitor<Element>} */
function inject(node) {
- const children = toChildren(content, node)
+ const children = toChildren(content || contentDefaults, node)
node.children[behavior === 'prepend' ? 'unshift' : 'push'](
create(node, toProperties(props, node), children)
)
@@ -146,7 +146,7 @@ export default function rehypeAutolinkHeadings(options) {
/* c8 ignore next -- uncommon */
if (typeof index !== 'number' || !parent) return
- const children = toChildren(content, node)
+ const children = toChildren(content || contentDefaults, node)
const link = create(node, toProperties(props, node), children)
let nodes = behavior === 'before' ? [link, node] : [node, link]
@@ -166,7 +166,19 @@ export default function rehypeAutolinkHeadings(options) {
/** @type {import('unist-util-visit').Visitor<Element>} */
function wrap(node) {
- node.children = [create(node, toProperties(props, node), node.children)]
+ let children = node.children
+
+ if (typeof content === 'function') {
+ const copy = content(node)
+ children = Array.isArray(copy) ? copy : [copy]
+ } else if (content) {
+ const copy = clone(content)
+ children = Array.isArray(copy)
+ ? [...node.children, ...copy]
+ : [...node.children, copy]
+ }
+
+ node.children = [create(node, toProperties(props, node), children)]
return [SKIP]
}
} |
Hello 👋 You welcome, thank you for reviewing this and all the stuff you do here, I spent quite some time recently with your packages and I love what can be done using them Oh wow, yes you are right, anchors in headings are problematic on github and they also don't work in the code I suggested
Yes they way you did it works great, I did a bunch of tests including with anchors The only thing is that if you have an anchor in the heading, then it will produce an anchor in anchor (which is invalid): /**
* @typedef {import('rehype-autolink-headings').Options} Options
*/
import {rehype} from 'rehype'
import rehypeAutolinkHeadings from 'rehype-autolink-headings'
const file = await rehype()
.data('settings', {fragment: true})
.use(
rehypeAutolinkHeadings,
/** @type {Options} */ {
behavior: 'wrap',
}
)
.process('<h1 id="foo">a <a href="c">b</a> d</h1>')
console.log(String(file)) will output: <h1 id="foo"><a href="#foo">a <a href="c">b</a> d</a></h1> but because content can be a function, the dev can solve this the way he wants by either doing a toString on the node or by use a more complex function that checks if there is an anchor in the children and if there is do something about it how do you prefer we proceed, do we close this PR and you commit your code or should I revert my changes and add your code, or I can do a completely new PR if this saves you time? |
Related-to: GH-20. Related-to: GH-21. Co-authored-by: Chris Weber <[email protected]>
This comment has been minimized.
This comment has been minimized.
Thanks! |
Thank you @wooorm for this new v7.1.0 release with improved wrap functionality, oh wow you also updated the docs and tests, nice 😃 I just tested it in my project, works like a charm |
Initial checklist
Description of changes
This PR is related to the ticket #20 I opened earlier, it serves as proof of concept for the idea I mentioned in the ticket and it shows what an impact the change would have on the current code base