Skip to content
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

Skip removing nodes in <code> #647

Merged
merged 2 commits into from Nov 23, 2020
Merged

Conversation

jakubriedl
Copy link
Contributor

Basically all syntax highlighters wrap comments in code blocks in <span class="comment">// some comment</span>.
And because word comment is correctly in REGEXPS.unlikelyCandidates and REGEXPS.negative they get removed. But those should be kept as it is valid content.

This PR fixes the issue by disabling removing of nodes in <code>.

test url: https://v8.dev/blog/emscripten-standalone-wasm

Copy link
Contributor

@gijsk gijsk left a comment

Choose a reason for hiding this comment

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

This looks good in principle, just one nit around the use of _hasAncestorTag. Thanks!

(And apologies for the slowness, things are rather... hectic... both at work and in my own life.)

Readability.js Outdated
@@ -2029,6 +2030,10 @@ Readability.prototype = {
return false;
}

if (this._hasAncestorTag(node, "code", -2)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why -2 ? In the other change you didn't pass a maxDepth argument at all. It also looks like passing 0 or -2 is functionally equivalent, so this is a bit confusing. Can you clarify, or maybe change to not pass this if it doesn't make a difference? Thanks!

@jakubriedl
Copy link
Contributor Author

nw with response time. Thank you for doing open-source projects.

I've removed the maxDepth param. There isn't actually any need to limit that. Was more copy/paste thing from line above.

@gijsk gijsk merged commit 290724c into mozilla:master Nov 23, 2020
@jakubriedl jakubriedl deleted the keep-code-comments branch November 23, 2020 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants