feat: add search root element #171
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
ylc395
force-pushed
the
feature/add-search-root-element
branch
from
fdbd7ea to
295d102
Compare
|
Thanks for the PR! This library is meant to polyfill URL Fragment Text Directives, and the algorithm for finding such text directives assumes to always search the entire document. I don't think specifying a different root makes sense given this context, but I might be missing the motivation for the PR. |
|
@tomayac I'm making an web app, which can process text fragment directives for a contained HTML document. Specifying the root element of the contained HTML would be helpful. |
|
To be compliant, the text fragment would need to be searched in the entire document. Is it that for your particular use case you can safely assume that the text fragment can only appear in a given part of the document? |
|
@tomayac Yes, let's assume that there is a web app for processing other HTML documents, and these HTML documents are treated independently. So we don't want to search text fragment in the main app (the main HTML document) |
tomayac
left a comment
tomayac
left a comment
There was a problem hiding this comment.
Just two suggestions, but after that LGTM
src/text-fragment-utils.js
Outdated
| * process. | ||
| * @param {Document} documentToProcess - document where to extract and mark | ||
| * fragments in. | ||
| * @param {Element} root - the root element where to extract and mark |
There was a problem hiding this comment.
| * @param {Element} root - the root element where to extract and mark | |
| * @param {Element=} root - the root element where to extract and mark |
There was a problem hiding this comment.
To mark the parameter as optional.
src/text-fragment-utils.js
Outdated
| * @param {TextFragment} textFragment - Text Fragment to highlight. | ||
| * @param {Document} documentToProcess - document where to extract and mark | ||
| * fragments in. | ||
| * @param {Element} root - the root element where to extract and mark |
There was a problem hiding this comment.
| * @param {Element} root - the root element where to extract and mark | |
| * @param {Element=} root - the root element where to extract and mark |
To mark the param as optional.
|
No problem |
|
Released as |
2 participants
add a
rootelement param forprocessFragmentDirectives