feat(highlighting): Can display textrange-based highlights #164

lukasbuenger · 2019-02-27T14:15:08Z

Relations:

Issue: livingdocsIO/livingdocs-planning/issues/2472
Related PR's:
- livingdocsIO/livingdocs-framework#378
- livingdocsIO/livingdocs-editor#2646

Changelog

API method highlight now takes an additional optional option argument textRange {start: number, end: number, text: string }. If given, it is used to determine the span position rather than the regex matches on text.
Added rangy's TextRange module.

Known issues

Formatting a selection that includes a comment edge will remove any comment span in the editable container
Splitting a comment destroys the highlight.
Splitting a component before a comment destroys the highlight.
Merging a component destroys all highlights in the second component.

Pull Request Checklist

Tests

src/selection.js

src/core.js

spec/highlighting.spec.js

src/selection.js

src/highlight-support.js

lukasbuenger · 2019-03-15T13:06:10Z

Closed for now in favor of #166

…tents

peyerluk · 2019-04-17T11:39:50Z

src/core.js

+   * @param  {String} options.text
+   * @param  {String} options.highlightId Added to the highlight markups in the property `data-word-id`
+   * @param  {Object} [options.textRange] An optional range which gets used to set the markers.
+   * @param  {Number} [options.textRange.start]


I think start and end are not optional if a textRange object is passed, right? So I think we can remove the square brackets around the param name here and in the next line.

peyerluk · 2019-04-17T11:42:50Z

spec/highlighting.spec.js


+  describe('highlightSupport', () => {
+    beforeEach(() => {
+      setupHighlightEnv(this, 'People Make The <br> World Go Round')


We should replace all arrow functions in this file with regular functions if you use this here. Otherwise this code is quite misleading.

(We could also do this in a separate PR for all tests in editable.js)

I would like to go with your second suggestion, as currently a lot of tests misbehave if you only just adjust the beforeEach scopings.

I would like to go with your second suggestion

That's ok

as currently a lot of tests misbehave if you only just adjust the beforeEach scopings.

This is because you have to change all methods in the same file at once, otherwise you mix scopes which does not work.

I did replace all of them to the point where I had now arrow functions at all anymore (at least in this spec), but still all hell broke loose. I'd rather do this with a little more focus and time.

peyerluk · 2019-04-17T12:28:49Z

spec/highlighting.spec.js

    })
  })

+  describe('highlightSupport', () => {


All these tests never test if we actually to the right thing! You only check that the index positions are the same, we do not know that the right characters are selected.

Tests now check for the text equivalent of the ranges. See also comment below.

peyerluk · 2019-04-17T12:38:09Z

src/core.js

+   *
+   * @param  {Object} options
+   * @param  {DOMNode} options.editableHos
+   * @return {Object} ranges


Why not include the text in the ranges? Otherwise how do you get the text of a range?

Why not include the text in the ranges?

Done. Good point. Will have to adjust specs in the framework as well. JFYI

peyerluk · 2019-04-18T14:54:57Z

src/highlighting.js

      throttle: 1000,
      // remove highlights after a change if the cursor is inside a highlight
-      removeOnCorrection: true,
+      removeOnCorrection: false,


Why did this change?

…t removal when mutating its content

lukasbuenger · 2019-04-23T13:18:12Z

spec/highlighting.spec.js

+          end: 9
+        }
+      }
+      const expectedHtml = this.formatHtml(`People M


Two closing spans. Why?

This was my typo. The test was still passing because of this

livingdocs-automation · 2019-04-24T13:26:19Z

🎉 This PR is included in version 1.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Merge: 2bcc82f 8541e62 Author: Lukas Bünger <lukasbuenger@gmail.com> Date: Wed Apr 24 15:23:00 2019 +0200 Merge pull request #164 from livingdocsIO/feat-highlight-range feat(highlighting): Can display textrange-based highlights

lukasbuenger requested a review from peyerluk February 27, 2019 15:14

houndci-bot reviewed Mar 4, 2019

View reviewed changes

src/selection.js Outdated Show resolved Hide resolved

src/core.js Outdated Show resolved Hide resolved

lukasbuenger commented Mar 7, 2019

View reviewed changes

spec/highlighting.spec.js Show resolved Hide resolved

lukasbuenger commented Mar 7, 2019

View reviewed changes

spec/highlighting.spec.js Show resolved Hide resolved

houndci-bot reviewed Mar 7, 2019

View reviewed changes

src/selection.js Show resolved Hide resolved

houndci-bot reviewed Mar 8, 2019

View reviewed changes

src/highlight-support.js Outdated Show resolved Hide resolved

lukasbuenger mentioned this pull request Mar 15, 2019

WIP Feat: VirtualSpan #166

Closed

1 task

lukasbuenger closed this Mar 15, 2019

lukasbuenger reopened this Mar 19, 2019

lukasbuenger added 18 commits March 19, 2019 10:28

feat(highlighting): Can display textrange-based highlights

18d4231

test: test text range highlighting

27d1905

refactor: expose rangy/TextRange as part of editable selection

423cd8a

refactor: introduce highlight span range extraction

99dffb9

test: highlight position extraction

26ec895

refactor: use a blank range to calculate text ranges

0f7e151

refactor: skip and warn when passed a cursor

1831748

refactor: respect newlines and upsert

6838b9e

refactor: wip, extend use case catalogue

be93cd6

refactor: export rangy text range as method

b1cfdbd

refactor: move anonymous import to top

38037db

test: make full example test the complete roundtrip

6dcd9eb

test: test with extracted ranges

22c6a08

fix: Selection.getTextRange respects newlines

796408d

refactor: use the text range module to extract and wrap highlight con…

47dee17

…tents

refactor: selection returns simple text range again

6525224

test: request 'natural' offsets

4d7efc5

fix: hound-cli error. remove unused fake match

9f5ee6b

lukasbuenger force-pushed the feat-highlight-range branch from f4b3005 to 9f5ee6b Compare March 19, 2019 09:31

fix: disable spellcheck sanitation

a95e4c5

fix: highlights get only applied to editableHost

f1d3535

peyerluk reviewed Apr 17, 2019

View reviewed changes

lukasbuenger added 4 commits April 18, 2019 16:06

chore: mark range props as required in doc block

1df1071

refactor: remove unused argumen

6727130

test: test text extraction based on extracted ranges as well

0cd0e93

feat: export the range text on the extracted range object

62b18d6

peyerluk reviewed Apr 18, 2019

View reviewed changes

lukasbuenger added 5 commits April 20, 2019 10:06

refactor: check for data-highlight value when deciding about highligh…

d2ef3eb

…t removal when mutating its content

fix: cleanup empty highlights

09e0bdb

test: html output

ef8b3f7

test: special chars

a010e9b

chore: rm package-lock.json, accidentially added.

cf60696

lukasbuenger requested a review from peyerluk April 20, 2019 11:03

lukasbuenger commented Apr 23, 2019

View reviewed changes

peyerluk approved these changes Apr 23, 2019

View reviewed changes

lukasbuenger added 2 commits April 24, 2019 11:51

test: small unit-tests for special chars in ranges

57387a4

test: check highlighting intersecting format tags

8541e62

lukasbuenger merged commit ef1e8a6 into master Apr 24, 2019

delete-merged-branch bot deleted the feat-highlight-range branch April 24, 2019 13:23

livingdocs-automation added the released label Apr 24, 2019

feat(highlighting): Can display textrange-based highlights #164

feat(highlighting): Can display textrange-based highlights #164

Uh oh!

Conversation

lukasbuenger commented Feb 27, 2019 • edited by Muhameti-centralsoft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

Known issues

Pull Request Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukasbuenger commented Mar 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukasbuenger Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

livingdocs-automation commented Apr 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lukasbuenger commented Feb 27, 2019 •

edited by Muhameti-centralsoft

Loading

lukasbuenger Apr 18, 2019 •

edited

Loading