feat(adapters): caret adapter basic implementation #58

neSpecc · 2023-11-18T00:10:07Z

Basic implementation of the Caret DOM Adapter

Binds "selectionchange" event on document using "useSelectionChange" util providing "on" and "off" methods.
"useSelectionChange" creates a singleton and delegates listeners
Caret Adapter provides attachInput and detachInput methods
Caret Adapter extends EventTarget and emits the change event — used for playground purposes. Could be removed later
Playground contains Input components with Caret Adapter attached. Caret index is displayed at the right side of the input

github-actions · 2023-11-18T00:10:45Z

✅ Mutation testing passed

Report: https://dashboard.stryker-mutator.io/reports/github.com/editor-js/document-model/PR-58

Mutated files

src/utils/EventBus/index.ts
src/utils/index.ts
src/index.ts

github-actions · 2023-11-18T00:11:17Z

Coverage report for `./packages/model`

St.^❔	Category	Percentage	Covered / Total
🟢	Statements	100%	589/589
🟢	Branches	99.27%	136/137
🟢	Functions	99.33%	149/150
🟢	Lines	100%	564/564

Test suite run success

347 tests passing in 20 suites.

Report generated by 🧪jest coverage report action from 4c73832

gohabereg · 2023-11-18T18:34:32Z

packages/dom-adapters/src/caret/CaretAdapter.ts

+/**
+ * Caret index is a tuple of start and end offset of a caret
+ */
+export type CaretIndex = [number, number];


We might re-use type from model

gohabereg · 2023-11-18T18:34:51Z

packages/dom-adapters/src/caret/CaretAdapter.ts

+  /**
+   * Index stores start and end offset of a caret depending on a root of input
+   */
+  #index: CaretIndex = [0, 0];


If caret is not set, what index it would be?

gohabereg · 2023-11-18T18:36:04Z

packages/dom-adapters/src/caret/CaretAdapter.ts

+   *
+   * @param selection - changed document selection
+   */
+  #onDocumentSelectionChange = (selection: Selection | null): void => {


We could try to optimise memory usage with a method (not a property). And pass the context from outside

gohabereg · 2023-11-18T18:36:35Z

packages/dom-adapters/src/caret/CaretAdapter.ts

+   * @param model - EditorJSModel instance
+   * @param blockIndex - index of a block that contains input
+   */
+  constructor(private readonly model: EditorJSModel, private readonly blockIndex: number) {


Would be better to use # modifier. It would add more lines, but properties would be "really" private

gohabereg · 2023-11-18T18:40:30Z

packages/dom-adapters/src/caret/CaretAdapter.ts

+   * @param input - input to watch caret change
+   * @param dataKey - key of data property in block's data that contains input's value
+   */
+  public attachInput(input: HTMLElement, dataKey: string): void {


You need an index of the block as well. Also dataKey is not used

block index passed to the constructor. It is not used right now as well as model and dataKey. I'll implement a working with model later

gohabereg · 2023-11-18T18:40:47Z

packages/dom-adapters/src/caret/CaretAdapter.ts

+   * Unsubscribes from input caret change
+   */
+  public detachInput(): void {
+    this.#offSelectionChange(this.#onDocumentSelectionChange);


You could set null to #input

gohabereg · 2023-11-18T18:41:22Z

packages/dom-adapters/src/caret/CaretAdapter.ts

+
+    const range = selection.getRangeAt(0);
+
+    return this.#input?.contains(range.startContainer) ?? false;


Ok for now, but we should think of cross-block selection case in the future

gohabereg · 2023-11-18T18:42:08Z

packages/dom-adapters/src/caret/utils/absoluteOffset.ts

+ *
+ * @param node - node to check
+ */
+function isLineBreak(node: Node): boolean {


Could you move it below the exported members

There will be ES Lint error: using of method before it declared

gohabereg · 2023-11-18T18:43:12Z

packages/dom-adapters/src/caret/utils/useSelectionChange.ts

+export const useSelectionChange = createSingleton(() => {
+  const subscribers = new Set<Subscriber>();
+
+  document.addEventListener('selectionchange', () => {


Do we need an API to remove the listener?

gohabereg · 2023-11-19T00:11:27Z

packages/dom-adapters/src/caret/CaretAdapter.ts

+  /**
+   * Returns absolute caret index related to input
+   */
+  public get index(): TextRange {


Should be above private methods

gohabereg · 2023-11-19T00:13:19Z

packages/dom-adapters/src/caret/utils/useSelectionChange.ts

+  /**
+   * Used to save context of the callback.
+   */
+  context: CaretAdapter;


It might be not only CaretAdapter in the future I suppose. Could be just unknown

gohabereg · 2023-11-19T00:14:15Z

packages/dom-adapters/src/caret/utils/useSelectionChange.ts

+
+    const range = selection.getRangeAt(0);
+
+    return input.contains(range.startContainer) ?? false;


Could you just add todo to think of cross-block selection later. To know where to look once we'll start working on it

You can actually use range.intersectsNode(input)

gohabereg · 2023-11-19T00:15:16Z

packages/dom-adapters/src/caret/utils/useSelectionChange.ts

+ */
+export const useSelectionChange = createSingleton(() => {
+  /**
+   * User to iterate over all inputs and check if selection is related to them.


Suggested change

* User to iterate over all inputs and check if selection is related to them.

* Used to iterate over all inputs and check if selection is related to them.

gohabereg · 2023-11-19T00:19:40Z

packages/dom-adapters/src/caret/utils/useSelectionChange.ts

+  /**
+   * WeakMap that stores subscribers for each input.
+   */
+  const subscribers = new WeakMap<InputWithCaret, Subscriber>();


Could there be more than one subscriber for an input?

gohabereg · 2023-11-19T00:21:25Z

packages/dom-adapters/src/caret/utils/useSelectionChange.ts

+  return {
+    on,
+    off,
+    destroy,


Once destroyed, there'll be no way to re-init as createSingletone will always return the same instance

I've exported init method as well. The destroy flow is not designed yet, so it will be implemented once we can understand it

feat(adapters): caret adapter basic implementation

568c0b1

fix naming

f6d84a2

gohabereg reviewed Nov 18, 2023

View reviewed changes

neSpecc added 4 commits November 18, 2023 19:57

use caret idx type from model

2566480

few fixed after review

26e6fc1

improve naming

e94c2e8

rm unused change

29e90ef

gohabereg reviewed Nov 19, 2023

View reviewed changes

neSpecc added 2 commits November 19, 2023 00:33

few fixes

256bdf0

make initial index is null instead of [0,0]

f0f6630

neSpecc requested a review from gohabereg November 19, 2023 00:36

gohabereg approved these changes Nov 19, 2023

View reviewed changes

throw error on multiple subscription

4c73832

neSpecc requested a review from ilyamore88 November 19, 2023 00:42

ilyamore88 approved these changes Nov 20, 2023

View reviewed changes

neSpecc added this pull request to the merge queue Nov 20, 2023

Merged via the queue into main with commit 38cfaa1 Nov 20, 2023
5 checks passed

neSpecc deleted the feat/caret-adapter branch November 20, 2023 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(adapters): caret adapter basic implementation #58

feat(adapters): caret adapter basic implementation #58

neSpecc commented Nov 18, 2023

github-actions bot commented Nov 18, 2023 •

edited

Loading

github-actions bot commented Nov 18, 2023 •

edited

Loading

gohabereg Nov 18, 2023

gohabereg Nov 18, 2023

gohabereg Nov 18, 2023

gohabereg Nov 18, 2023

gohabereg Nov 18, 2023

neSpecc Nov 18, 2023

gohabereg Nov 18, 2023

gohabereg Nov 18, 2023

gohabereg Nov 18, 2023

neSpecc Nov 18, 2023

gohabereg Nov 18, 2023

gohabereg Nov 19, 2023

gohabereg Nov 19, 2023

gohabereg Nov 19, 2023

gohabereg Nov 19, 2023

gohabereg Nov 19, 2023

gohabereg Nov 19, 2023

neSpecc Nov 19, 2023

gohabereg Nov 19, 2023

neSpecc Nov 19, 2023

neSpecc Nov 19, 2023


		const range = selection.getRangeAt(0);

		return this.#input?.contains(range.startContainer) ?? false;


		const range = selection.getRangeAt(0);

		return input.contains(range.startContainer) ?? false;

	* User to iterate over all inputs and check if selection is related to them.
	* Used to iterate over all inputs and check if selection is related to them.

feat(adapters): caret adapter basic implementation #58

feat(adapters): caret adapter basic implementation #58

Conversation

neSpecc commented Nov 18, 2023

github-actions bot commented Nov 18, 2023 • edited Loading

✅ Mutation testing passed

github-actions bot commented Nov 18, 2023 • edited Loading

Coverage report for ./packages/model

Test suite run success

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 18, 2023 •

edited

Loading

github-actions bot commented Nov 18, 2023 •

edited

Loading

Coverage report for `./packages/model`