feat(dom-adapters): Initial InlineToolAdapter implementation #59
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
❌ Mutation testing hasn't passed score thresholdReport: https://dashboard.stryker-mutator.io/reports/github.com/editor-js/document-model/PR-59 Mutated filessrc/utils/EventBus/index.ts |
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
|---|---|---|---|
| 🟢 | Statements | 100% | 579/579 |
| 🟢 | Branches | 99.27% | 136/137 |
| 🟢 | Functions | 99.33% | 149/150 |
| 🟢 | Lines | 100% | 554/554 |
Test suite run failed
Failed tests: 5/347. Failed suites: 2/20.
● ParentInlineNode › .format() › should emit TextFormattedEvent with inline fragment as data and affected range as index
expect(received).toBeInstanceOf(expected)
Expected constructor: TextFormattedEvent
Received value has no prototype
Received value: null
486 | node.format(tool, start, end, data);
487 |
> 488 | expect(event).toBeInstanceOf(TextFormattedEvent);
| ^
489 | expect(event).toHaveProperty('detail', expect.objectContaining({
490 | action: EventAction.Modified,
491 | index: [ [start, end] ],
at Object.<anonymous> (src/entities/inline-fragments/specs/ParentInlineNode.spec.ts:488:21)
● FormattingInlineNode › .unformat() › should split node into tree if unformatting applied in the middle of the node
TypeError: Method Map.prototype.entries called on incompatible receiver [object Map]
234 | const result = node.unformat(tool, start, end);
235 |
> 236 | expect(result).toEqual([
| ^
237 | expect.any(FormattingInlineNode),
238 | expect.any(TextInlineNode),
239 | expect.any(FormattingInlineNode),
at Object.<anonymous> (src/entities/inline-fragments/specs/FormattingInlineNode.spec.ts:236:22)
● FormattingInlineNode › .unformat() › should remove unformatted node from parent if unformatting applied in the middle of the node
expect(received).toHaveLength(expected)
Expected length: 2
Received length: 3
Received array: [{"data": {}, "tool": "bold", Symbol(kEvents): Map {}, Symbol(events.maxEventTargetListeners): 10, Symbol(events.maxEventTargetListenersWarned): false, Symbol(kHandlers): Map {}}, {"value": "to"}, {"data": {}, "tool": "bold", Symbol(kEvents): Map {}, Symbol(events.maxEventTargetListeners): 10, Symbol(events.maxEventTargetListenersWarned): false, Symbol(kHandlers): Map {}}]
244 | const result = node.unformat(tool, start, end);
245 |
> 246 | expect(parent.children).toHaveLength(result.length);
| ^
247 | });
248 |
249 | it('should split node into two if unformatting applied at the end of the node', () => {
at Object.<anonymous> (src/entities/inline-fragments/specs/FormattingInlineNode.spec.ts:246:31)
● FormattingInlineNode › .unformat() › should split node into two if unformatting applied at the end of the node
expect(received).toEqual(expected) // deep equality
- Expected - 2
+ Received + 3
Array [
- Any<<anonymous>>,
- Any<ChildNode>,
+ ChildNode {
+ "value": "r.js is a block-styled editor",
+ },
]
250 | const result = node.unformat(tool, end, node.length);
251 |
> 252 | expect(result).toEqual([
| ^
253 | expect.any(FormattingInlineNode),
254 | expect.any(TextInlineNode),
255 | ]);
at Object.<anonymous> (src/entities/inline-fragments/specs/FormattingInlineNode.spec.ts:252:22)
● FormattingInlineNode › .unformat() › should remove unformatted node from parent if unformatting applied at the end of the node
expect(received).toHaveLength(expected)
Expected length: 1
Received length: 2
Received array: [{"data": {}, "tool": "bold", Symbol(kEvents): Map {}, Symbol(events.maxEventTargetListeners): 10, Symbol(events.maxEventTargetListenersWarned): false, Symbol(kHandlers): Map {}}, {"value": "r.js is a block-styled editor"}]
259 | const result = node.unformat(tool, end, node.length);
260 |
> 261 | expect(parent.children).toHaveLength(result.length);
| ^
262 | });
263 | });
264 |
at Object.<anonymous> (src/entities/inline-fragments/specs/FormattingInlineNode.spec.ts:261:31)
Report generated by 🧪jest coverage report action from 098d37a
neSpecc
reviewed
Nov 19, 2023
|
|
||
| const range = selection?.getRangeAt(0); | ||
|
|
||
| const comparison = range.compareBoundaryPoints(Range.START_TO_END, range); |
There was a problem hiding this comment.
pls improve variable naming. Now it is not clear what is stored in there
| const tool = this.#tools.get(data.tool); | ||
| const wrappedTool = tool.create(data.data); | ||
|
|
||
| wrappedTool.dataset.tool = data.tool; |
There was a problem hiding this comment.
add js doc pls describing the reason why we store this data
| * | ||
| * @param node | ||
| */ | ||
| export function getLength(node: Node): number { |
There was a problem hiding this comment.
Suggested change
| export function getLength(node: Node): number { | |
| export function getNodeTextLength(node: Node): number { |
| * | ||
| * @returns [child, offset] - child node and offset relative to the child node | ||
| */ | ||
| function findChildByIndex(node: Node, index: number): [child: Node, offset: number] { |
There was a problem hiding this comment.
Suggested change
| function findChildByIndex(node: Node, index: number): [child: Node, offset: number] { | |
| function findChildByCharIndex(node: Node, index: number): [child: Node, offset: number] { |
| * @param start - start char index | ||
| * @param end - end char index | ||
| */ | ||
| export function getRange(input: HTMLElement, start: number, end: number): Range { |
There was a problem hiding this comment.
Suggested change
| export function getRange(input: HTMLElement, start: number, end: number): Range { | |
| export function createRange(input: HTMLElement, start: number, end: number): Range { |
| let nextSibling = startContainer.nextSibling; | ||
| let parent = startContainer.parentNode!; | ||
|
|
||
| while (nextSibling === null && parent !== input) { |
There was a problem hiding this comment.
Suggested change
| while (nextSibling === null && parent !== input) { | |
| while (nextSibling === null && parent !== input && parent !== null) { |
Comment on lines
143
to
157
| let nextSibling = endContainer.nextSibling; | ||
| let parent = endContainer.parentNode!; | ||
|
|
||
| while (nextSibling === null && parent !== input) { | ||
| nextSibling = parent.nextSibling; | ||
| parent = parent.parentNode!; | ||
| } | ||
|
|
||
| if (nextSibling !== null) { | ||
| range.setEndBefore(nextSibling); | ||
| } else { | ||
| range.setEndAfter(endContainer); | ||
| } | ||
| } else { | ||
| range.setEnd(endContainer, endOffset); |
There was a problem hiding this comment.
seems like this code could me moved to some method or util since it repeated above
| /** | ||
| * To cover the first case, we need to check if there are any closest elements with targetTool relative to common ancestor | ||
| */ | ||
| const targetToolAbove = commonAncestorElement.closest(`[data-tool="${targetTool}"]`) as HTMLElement | null; |
There was a problem hiding this comment.
what does it mean "Above" and "Below" in this context?
|
update a branch please |
|
⏭️ No files to mutate for |
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
|---|---|---|---|
| 🟢 | Statements | 100% | 0/0 |
| 🟢 | Branches | 100% | 0/0 |
| 🟢 | Functions | 100% | 0/0 |
| 🟢 | Lines | 100% | 0/0 |
Test suite run success
1 tests passing in 1 suite.
Report generated by 🧪jest coverage report action from 7f06e0f
|
⏭️ No files to mutate for |
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
|---|---|---|---|
| 🟢 | Statements | 100% | 579/579 |
| 🟢 | Branches | 99.27% | 136/137 |
| 🟢 | Functions | 99.33% | 149/150 |
| 🟢 | Lines | 100% | 554/554 |
Test suite run success
348 tests passing in 20 suites.
Report generated by 🧪jest coverage report action from 7f06e0f
neSpecc
reviewed
Dec 7, 2023
| } | ||
|
|
||
| public applyFormat(toolName: InlineToolName, data: InlineToolData): void { | ||
| const index = this.#caretAdapter.getIndex(); |
There was a problem hiding this comment.
maybe index should be passed as an argument?
| /** | ||
| * Remove formatting from the input | ||
| */ | ||
| if (event instanceof TextUnformattedEvent) { |
| return acc + child.textContent!.length; | ||
| }, offset); | ||
| return acc + getNodeTextLength(child); | ||
| }, initialNode instanceof Text ? offset : 0); |
There was a problem hiding this comment.
comment will be helpful for a future
| * | ||
| * @param node - node to normalize | ||
| */ | ||
| export function normalizeNode(node: Node): void { |
There was a problem hiding this comment.
Suggested change
| export function normalizeNode(node: Node): void { | |
| export function normalizeNodeByDataTool(node: Node): void { |
| /** | ||
| * To cover the first case, we need to check if there are any closest elements with targetTool relative to common ancestor | ||
| */ | ||
| const targetToolAbove = commonAncestorElement.closest(`[data-tool="${targetTool}"]`) as HTMLElement | null; |
There was a problem hiding this comment.
Many utility functions relies on data-tool attribute, which is a prat of business logic. Maybe we can do something with that implicit dependency
| @@ -243,22 +242,21 @@ describe('FormattingInlineNode', () => { | |||
| it('should remove unformatted node from parent if unformatting applied in the middle of the node', () => { | |||
| const result = node.unformat(tool, start, end); | |||
|
|
|||
| expect(parent.children).toHaveLength(result.length); | |||
| expect(parent.children).toHaveLength(result.length + 1); | |||
There was a problem hiding this comment.
please, update the test case name to describe this statement o leave a jsdoc
| range, | ||
| }; | ||
| }, | ||
| } satisfies InlineTool; |
There was a problem hiding this comment.
how tool can declare the data that should return on save? For example, color
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.