-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat(dom-adapters): basic inline tool adapter implementation #74
Conversation
…re/global-caret-adapter
…re/global-caret-adapter
…to inline-tool-adapter
⏭️ 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 6db4d90
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
---|---|---|---|
🟢 | Statements | 100% | 753/753 |
🟢 | Branches | 99.5% (+0.01% 🔼) |
201/202 |
🟢 | Functions | 98.37% (-1.07% 🔻) |
181/184 |
🟢 | Lines | 100% | 725/725 |
Show new covered files 🐣
St.❔ |
File | Statements | Branches | Functions | Lines |
---|---|---|---|---|---|
🟢 | ... / IntersectType.ts |
100% | 100% | 100% | 100% |
🟢 | ... / FormattingAction.ts |
100% | 100% | 100% | 100% |
Show files with reduced coverage 🔻
St.❔ |
File | Statements | Branches | Functions | Lines |
---|---|---|---|---|---|
🟢 | ... / index.ts |
100% | 100% | 50% (-50% 🔻) |
100% |
Test suite run success
389 tests passing in 24 suites.
Report generated by 🧪jest coverage report action from 6db4d90
⏭️ No files to mutate for |
|
||
const inlineElement = tool.create(); | ||
|
||
range.surroundContents(inlineElement); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would throw an exception in some cases, see https://developer.mozilla.org/en-US/docs/Web/API/Range/surroundContents
const textRange = index.textRange!; | ||
const blockIndex = index.blockIndex!; | ||
const dataKey = index.dataKey!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-null assertion could be dangerous here, would be better to check each index part
|
||
const fragments = this.#model.getFragments(blockIndex, dataKey, ...textRange, toolName); | ||
|
||
const { action, range } = tool.getAction(textRange, fragments, intersectType, data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tool will know it's intersect type by itself, no need to pass it here. It should be passed to the model when we'll implement different intersections support
* @returns {boolean} true if itersect exists, false otherwise | ||
*/ | ||
export function intersectionExists(firstRange: TextRange, secondRange: TextRange): boolean { | ||
return (firstRange[0] < secondRange[1] && firstRange[0] > secondRange[0] || firstRange[1] < secondRange[1] && firstRange[1] > secondRange[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use a formula
(firstRange[0] - secondRange[1]) * (secondRange[0] - firstRange[1]) > 0
* @param secondRange - second text range | ||
* @returns {boolean} true if itersect exists, false otherwise | ||
*/ | ||
export function intersectionExists(firstRange: TextRange, secondRange: TextRange): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also let's rename to smth like doRangesIntersect
* @param secondRange - second range to be merged | ||
* @returns {TextRange} merged text range | ||
*/ | ||
export function mergeTextRanges(firstRange: TextRange, secondRange: TextRange): TextRange { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to check if ranges intersects right inside this function
if (selection) { | ||
/** | ||
* Do not render inline toolbar for not contenteditable elements | ||
*/ | ||
if (selection.focusNode?.nodeType !== Node.TEXT_NODE) { | ||
return; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be enough to check if caret has textRange. If it has it — that means caret is in the text input
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as i can see, caret also has text range for native inputs, but i want to check that current node is text and contenteditable
* Class handles on format model events and renders inline tools | ||
* Applies format to the model | ||
*/ | ||
export class InlineToolAdapter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it would be a single adapter for all of the tools, let's name it InlineToolsAdapter
packages/model/src/entities/inline-fragments/FormattingInlineNode/types/FormattingAction.ts
Show resolved
Hide resolved
/** | ||
* Formatting action - format or unformat | ||
*/ | ||
action: FormattingAction; | ||
|
||
/** | ||
* Range to apply formatting action | ||
*/ | ||
range: TextRange; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's extract this to a separate intereface
const selection = window.getSelection(); | ||
|
||
/** | ||
* Get current input with selection | ||
*/ | ||
if (selection) { | ||
/** | ||
* Do not render inline toolbar for not contenteditable elements | ||
*/ | ||
if (selection.focusNode?.nodeType !== Node.TEXT_NODE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left same comment previously, you should be able to just check if index has a text range, no need to check node type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated comments
/** | ||
* If two fragments of one tool intersect - treat them as two different fragments | ||
*/ | ||
Separate = 'separate' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like that name, I think it's not descriptive enough. Maybe something like LeaveBoth
or None
as there's no intersection in that case. Happy to discuss other ideas
/** | ||
* If two fragments of one tool intersect - treat them as two different fragments | ||
*/ | ||
LeaveBoth = 'LeaveBoth' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent
LeaveBoth = 'LeaveBoth' | |
LeaveBoth = 'leave-both' |
Inline tool adapter implementation
structure:
Editor
(playground simulation)InlineTool
instanse (one for editor, for each inline tool)InlineToolbar
toolbar that is rendered with inline tools available inside blockInlineToolbar
class)Toolbar
componentInlineToolAdapter
- adapter, that connects model and editormodel
stores dataNOTE:
Progress
InlineToolbar
InlineToolAdapter
action
andrange
from tool instanceintersectType
for inline toolsNext
IntersectType
for inline tools