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

Parse only content necessary to mentionify doc #59

Merged
merged 4 commits into from
May 2, 2018

Conversation

ashmaroli
Copy link
Member

@ashmaroli ashmaroli commented Mar 21, 2018

Instead of having Nokogiri parse the entire HTML doc and then selectively apply the mentionify_filter to just the contents of the <body> tag, let's have Nokogiri parse just the <body> tag descendants instead

The mentionified markup is then inserted back into doc.output via String#<<

Fixes #37 as well

Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -8,24 +8,27 @@ class Mentions
GITHUB_DOT_COM = "https://github.com".freeze
BODY_START_TAG = "<body".freeze

OPENING_BODY_TAG_REGEX = %r!<body(.*)>\s*!
Copy link
Contributor

Choose a reason for hiding this comment

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

Same questions as jekyll/jemoji#74 (comment)... cross-posting theme here for ease of reference:

  1. Does the .* need to be non-greedy? (not sure which is more fault tolerant)
  2. Does this need to be case insensitive to allow for <BODY tags?
  3. Could we simplify/collapse the existing logic with BODY_START_TAG?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ashmaroli ashmaroli force-pushed the parse-html-fragment branch 2 times, most recently from 14be2e2 to aeffa1c Compare April 25, 2018 19:10
@DirtyF
Copy link
Member

DirtyF commented May 2, 2018

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 3cac982 into jekyll:master May 2, 2018
jekyllbot added a commit that referenced this pull request May 2, 2018
@ashmaroli ashmaroli deleted the parse-html-fragment branch May 2, 2018 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants