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

#23 Removed trimRange usage #67

Merged

Conversation

oleksandr-danylchenko
Copy link
Contributor

Issue

This PR covers the issue #66. Where I expressed my doubts about whether we need a trimRange in the selection range processing flow. Two main points that shifted me towards the "No" answer are:

  1. User can intentionally select non-textual nodes. Taking them out of the selection range only when they are at the edges, but not in the middle, seems like a partial prevention of a behavior
  2. Ranges that have the start/endContainer.nodeType different from the Node.TEXT_NODE still can contain only the textual annotatable content. And such ranges are usually created when using the .setStartAfter/. setEndBefore

But to grasp the whole picture you can get acquainted with the detailed description and thought process here - #66 (comment)

@oleksandr-danylchenko oleksandr-danylchenko marked this pull request as draft February 27, 2024 14:25
@oleksandr-danylchenko

This comment was marked as outdated.

@oleksandr-danylchenko oleksandr-danylchenko marked this pull request as ready for review February 27, 2024 14:45
@rsimon
Copy link
Member

rsimon commented Feb 27, 2024

The trimRange function is really a remnant of the previous version of the text annotator or, in fact, even the previous version's predecessor (which worked in a "controlled environment" of a single DIV). Back then, the goal was simply to remove trailing and leading spaces.

I extended it later so that highlight SPANs would get inserted only across the text nodes. But, obviously, that was a very different rendering approach back then.

I'll review this PR when I have the time, but I believe trimRange is indeed not needed any more.

@oleksandr-danylchenko
Copy link
Contributor Author

oleksandr-danylchenko commented Feb 27, 2024

Thank you very much for the feedback 🙏🏻
The doubts of me just using the library in some unintended way were tearing me apart. And another portion of doubts aobut submitting such a PR would be considered as a super bold move 😅

@rsimon rsimon merged commit 923bf64 into recogito:main Jun 12, 2024
@oleksandr-danylchenko oleksandr-danylchenko deleted the #23-trim-range-creates-collapsed branch June 12, 2024 16:47
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