You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@annotator.apache.org by GitBox <gi...@apache.org> on 2021/06/04 19:17:51 UTC
[GitHub] [incubator-annotator] Treora opened a new pull request #110: Allow passing a Node as scope in dom functions
Treora opened a new pull request #110:
URL: https://github.com/apache/incubator-annotator/pull/110
Addresses issue #109.
This makes the functions easier to use for the usual case where the scope is a Node (e.g. the whole document, or `document.body`).
Each function internally just casts the scope’s `Node | Range` type to a `Range`, and continues to work with that.
Return types are left unchanged, except that the `CssSelector` matcher now returns an `Element`, instead of creating a `Range` that selects that element’s contents. For users that just want to use `CssSelector`s, this would make a lot more sense; and it simplifies our code. As all other functions now accept a Node as their scope, refinement still works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-annotator] tilgovi commented on a change in pull request #110: Allow passing a Node as scope in dom functions
Posted by GitBox <gi...@apache.org>.
tilgovi commented on a change in pull request #110:
URL: https://github.com/apache/incubator-annotator/pull/110#discussion_r645887991
##########
File path: packages/dom/src/css.ts
##########
@@ -45,21 +46,17 @@ import { ownerDocument } from './owner-document';
* > “If […] the user agent discovers multiple matching text sequences, then the
* > selection SHOULD be treated as matching all of the matches.”
*
- * Each matching element is returned as a {@link https://developer.mozilla.org/en-US/docs/Web/API/Range
- * | Range} surrounding that element. This in order to make its output reusable
- * as the scope for any subsequents selectors that {@link
- * Selector.refinedBy | refine} this CssSelector.
- *
- * @param selector - The {@link CssSelector} to be anchored
- * @returns A {@link Matcher} function that applies `selector` to a given {@link https://developer.mozilla.org/en-US/docs/Web/API/Range
- * | Range}
+ * @param selector - The {@link CssSelector} to be anchored.
+ * @returns A {@link Matcher} function that applies `selector` to a given
+ * `scope`.
*
* @public
*/
export function createCssSelectorMatcher(
selector: CssSelector,
-): Matcher<Range, Range> {
+): Matcher<Node | Range, Element> {
Review comment:
Should the result be `Node` or `Element`? Should the input be `Node`? I think `ParentNode` is actually part of the DOM libdefs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-annotator] Treora commented on a change in pull request #110: Allow passing a Node as scope in dom functions
Posted by GitBox <gi...@apache.org>.
Treora commented on a change in pull request #110:
URL: https://github.com/apache/incubator-annotator/pull/110#discussion_r646008042
##########
File path: packages/dom/src/range-node-conversion.ts
##########
@@ -0,0 +1,45 @@
+/**
Review comment:
I thought about it, but it’s seems confusing with RangeSelector! (which now lives in a folder called `range`, so besides confusion it might create ambiguity).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-annotator] tilgovi commented on a change in pull request #110: Allow passing a Node as scope in dom functions
Posted by GitBox <gi...@apache.org>.
tilgovi commented on a change in pull request #110:
URL: https://github.com/apache/incubator-annotator/pull/110#discussion_r645888087
##########
File path: packages/dom/src/css.ts
##########
@@ -45,21 +46,17 @@ import { ownerDocument } from './owner-document';
* > “If […] the user agent discovers multiple matching text sequences, then the
* > selection SHOULD be treated as matching all of the matches.”
*
- * Each matching element is returned as a {@link https://developer.mozilla.org/en-US/docs/Web/API/Range
- * | Range} surrounding that element. This in order to make its output reusable
- * as the scope for any subsequents selectors that {@link
- * Selector.refinedBy | refine} this CssSelector.
- *
- * @param selector - The {@link CssSelector} to be anchored
- * @returns A {@link Matcher} function that applies `selector` to a given {@link https://developer.mozilla.org/en-US/docs/Web/API/Range
- * | Range}
+ * @param selector - The {@link CssSelector} to be anchored.
+ * @returns A {@link Matcher} function that applies `selector` to a given
+ * `scope`.
*
* @public
*/
export function createCssSelectorMatcher(
selector: CssSelector,
-): Matcher<Range, Range> {
+): Matcher<Node | Range, Element> {
Review comment:
I guess the result is always `Element`, so keep that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-annotator] tilgovi commented on a change in pull request #110: Allow passing a Node as scope in dom functions
Posted by GitBox <gi...@apache.org>.
tilgovi commented on a change in pull request #110:
URL: https://github.com/apache/incubator-annotator/pull/110#discussion_r645887098
##########
File path: packages/dom/src/range-node-conversion.ts
##########
@@ -0,0 +1,45 @@
+/**
Review comment:
Could just call this file `range.ts`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-annotator] Treora merged pull request #110: Allow passing a Node as scope in dom functions
Posted by GitBox <gi...@apache.org>.
Treora merged pull request #110:
URL: https://github.com/apache/incubator-annotator/pull/110
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-annotator] Treora commented on a change in pull request #110: Allow passing a Node as scope in dom functions
Posted by GitBox <gi...@apache.org>.
Treora commented on a change in pull request #110:
URL: https://github.com/apache/incubator-annotator/pull/110#discussion_r646008519
##########
File path: packages/dom/src/range-node-conversion.ts
##########
@@ -0,0 +1,45 @@
+/**
Review comment:
I may just call it `to-range.ts` for now, as that the name of its only export. (I started coding the inverse as well, `toNode`, hence I had opted for a more generic name)
I would be fine with throwing `ownerDocument` into the same file, if you like.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-annotator] tilgovi commented on a change in pull request #110: Allow passing a Node as scope in dom functions
Posted by GitBox <gi...@apache.org>.
tilgovi commented on a change in pull request #110:
URL: https://github.com/apache/incubator-annotator/pull/110#discussion_r645887503
##########
File path: packages/dom/src/css.ts
##########
@@ -45,21 +46,17 @@ import { ownerDocument } from './owner-document';
* > “If […] the user agent discovers multiple matching text sequences, then the
* > selection SHOULD be treated as matching all of the matches.”
*
- * Each matching element is returned as a {@link https://developer.mozilla.org/en-US/docs/Web/API/Range
- * | Range} surrounding that element. This in order to make its output reusable
- * as the scope for any subsequents selectors that {@link
- * Selector.refinedBy | refine} this CssSelector.
- *
- * @param selector - The {@link CssSelector} to be anchored
- * @returns A {@link Matcher} function that applies `selector` to a given {@link https://developer.mozilla.org/en-US/docs/Web/API/Range
- * | Range}
+ * @param selector - The {@link CssSelector} to be anchored.
+ * @returns A {@link Matcher} function that applies `selector` to a given
+ * `scope`.
*
* @public
*/
export function createCssSelectorMatcher(
selector: CssSelector,
-): Matcher<Range, Range> {
+): Matcher<Node | Range, Element> {
Review comment:
This is the only part that worries me. It feels a little like this should be symmetrical for some reason, but I'll think more on it later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org