Skip to content

fix: Only highlight first term after heading [INGEST-1262] #5346

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

Merged
merged 4 commits into from
Jul 28, 2022
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 37 additions & 24 deletions src/gatsby/plugins/gatsby-remark-terms/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,23 +24,30 @@ const REGEX = new RegExp(`(\\b|\\W)(${PATTERN})(\\b|\\W)`);
const CUSTOM_LINK_START = new RegExp("^<([a-zA-Z]+Link|a) ");
const CUSTOM_LINK_END = new RegExp("^</([a-zA-Z]+Link|a)>");

function replace(node) {
function replace(state, node) {
if (node.type == "root") return;

// If this is an empty node there's nothing to consider.
if (!node.children) return;
if (!node.children) return "skip";

// Do not replace abbreviations in headings because that appears to break the heading anchors.
if (node.type == "heading") return;
if (node.type == "heading") {
state.alreadyExplained = {};
return "skip";
}

// Do not replace abbreviations in links because that's two interactive
// nested elements nested in each other, and we decided we don't want to
// style that either.
//
// This currently doesn't handle nesting of e.g.
// <a><strong><abbr>... but we don't have that in docs.
if (node.type == "link") return;
if (node.type == "link") return "skip";

let insideCustomLink = false;

const newChildren = [];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I made the initial change, the build never terminated. I had to run Instruments.app against gatsby to figure that most of the time was spent in array splicing. Fixing this gross misuse of datastructures made the build terminate again.

It seems that the second-heaviest stacktrace is about regex replacements. Will create a followup PR.

Copy link
Member Author

@untitaker untitaker Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sike, the thing i wanted to do doesn't actually have an impact (explicit precompiling of regexes). I think we would want to spend some time thinking about how to make gatsby-remark-variables faster, which I think is dominating our build times. cc @dcramer


// If a text node is present in child nodes, check if an abbreviation is present
for (let c = 0; c < node.children.length; c++) {
const child = node.children[c];
Expand Down Expand Up @@ -85,36 +92,42 @@ function replace(node) {
}
}

if (insideCustomLink) continue;
if (child.type !== "text") continue;
if (!REGEX.test(child.value)) continue;
if (insideCustomLink || child.type !== "text" || !REGEX.test(child.value)) {
newChildren.push(child);
continue;
}

// Transform node
const newTexts = child.value.split(REGEX);

// Remove old text node
node.children.splice(c, 1);

// Replace abbreviations
for (let i = 0; i < newTexts.length; i++) {
const content = newTexts[i];
node.children.splice(
c + i,
0,
TERMS[content]
? {
type: "html",
value: `<div class="term-wrapper"><span class="term">${content}</span><span class="description" role="tooltip" aria-label="${content} definition">${TERMS[content]}</span></div>`,
}
: {
type: "text",
value: content,
}
);
let newNode;
if (TERMS[content] && !state.alreadyExplained[content]) {
state.alreadyExplained[content] = true;
newNode = {
type: "html",
value: `<div class="term-wrapper"><span class="term">${content}</span><span class="description" role="tooltip" aria-label="${content} definition">${TERMS[content]}</span></div>`,
};
} else {
newNode = {
type: "text",
value: content,
};
}

newChildren.push(newNode);
}
}

node.children = newChildren;
}

module.exports = async ({ markdownAST }) => {
visit(markdownAST, () => true, replace);
const state = {
alreadyExplained: {},
};
const replaceWithState = replace.bind(null, state);
visit(markdownAST, () => true, replaceWithState);
};