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

Conversation

untitaker
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Jul 27, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
sentry-docs ✅ Ready (Inspect) Visit Preview Jul 28, 2022 at 9:58AM (UTC)

@untitaker untitaker requested review from stevenplewis and a team July 27, 2022 15:12

let insideCustomLink = false;

const alreadyExplained = {};

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

@imatwawana
Copy link
Contributor

In one of our discussions, you suggested highlighting the first instance after each heading, and I thought that was a good idea, so that's what I thought we were doing. Is that a hard change to make at this point?

Also, I didn't realize that doing this based on paragraphs would mean that any other type of text (e.g., text in lists or tables) wouldn't use the highlighting (see the word "quota"):
https://sentry-docs-git-fix-terms-first-in-paragraph.sentry.dev/product/accounts/quotas/
Would this be different if we went with first instance after each heading?

@untitaker
Copy link
Member Author

so that's what I thought we were doing. Is that a hard change to make at this point?

I completely forgot, I was messing around with multiple versions of this due to performance problems, but yeah that's what we agreed on. Fixed.

Also, I didn't realize that doing this based on paragraphs would mean that any other type of text (e.g., text in lists or tables) wouldn't use the highlighting (see the word "quota"):

That's a bug I introduced, also fixed.

Copy link
Contributor

@imatwawana imatwawana left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for getting this into the quarter!

@untitaker untitaker merged commit f341d2e into master Jul 28, 2022
@untitaker untitaker deleted the fix/terms-first-in-paragraph branch July 28, 2022 13:11
@imatwawana imatwawana changed the title fix: Only highlight first term in paragraph [INGEST-1262] fix: Only highlight first term after heading [INGEST-1262] Jul 28, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Aug 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants