Backspace handler: Merge blocks #254
Conversation
| /** | ||
| * Merge two blocks | ||
| * @param {Block} targetBlock - block to merge | ||
| * @param {Block} mergingBlock - block that will be merged with target block |
| extractedBlock = range.extractContents(); | ||
|
|
||
| targetBlock.pluginsContent.appendChild(extractedBlock); | ||
| targetBlock.pluginsContent.normalize(); |
There was a problem hiding this comment.
А никак не сделать это без прямых манипуляций с DOM-деревом?
src/components/dom.js
Outdated
| * Check passed node if it has IMG, Twitter, iframe or something that could contain media | ||
| * @param target | ||
| */ | ||
| static hasMediaContent(target) { |
There was a problem hiding this comment.
уже есть, см класс Block@hasMedia
| @@ -185,6 +185,65 @@ export default class BlockManager extends Module { | |||
|
|
|||
| } | |||
|
|
|||
| /** | |||
| * Merge two blocks | |||
| * @param {Block} targetBlock - block to merge | |||
There was a problem hiding this comment.
block to merge это название второго параметра. Нужно внести ясность
|
|
||
| } | ||
|
|
||
| if (!$.isEmpty(blockToMerge .html)) { |
There was a problem hiding this comment.
и для проверки пустоты у класса Block есть метод isEmpty
|
|
||
| if (!$.isEmpty(blockToMerge .html)) { | ||
|
|
||
| let selection = Selection.get(), |
There was a problem hiding this comment.
возможно тебе подойдет метод
this.Editor.Caret.extractFragmentFromCaretPosition()
| this.currentNode = this._blocks[this.currentBlockIndex].html; | ||
|
|
||
| // set caret to the block without offset at the end | ||
| this.Editor.Caret.setToBlock(this.currentBlock, 0, true); |
There was a problem hiding this comment.
каретка и тулбар не относятся к removeBlock, нужно перенести их в функцию, которая обрабатывает конкретное действие, например backspace
src/components/modules/keyboard.js
Outdated
| @@ -93,11 +94,36 @@ export default class Keyboard extends Module { | |||
|
|
|||
| } | |||
|
|
|||
| if (this.Editor.Caret.isAtEnd) { | |||
There was a problem hiding this comment.
это точно необходимо? В центре блока тоже должен работать Сплит
| this._blocks.remove(index); | ||
|
|
||
| // decrease current block index so that to know current actual | ||
| this.currentBlockIndex--; |
| * Call plugins merge method | ||
| * @param {Object} data | ||
| */ | ||
| mergeWith(data) { |
src/components/block.js
Outdated
| mergeWith(data) { | ||
|
|
||
| return Promise.resolve() | ||
| .then( () => { |
There was a problem hiding this comment.
Можно сразу в resolve() функцию передать
There was a problem hiding this comment.
Или так
Promise.resolve(data).then(this.tool.merge)
| if (blockToMerge.isEmpty) { | ||
|
|
||
| this.removeBlock(blockToMergeIndex); | ||
| return Promise.resolve(); |
There was a problem hiding this comment.
Если сделать функцию асинхронной, можно не возвращать Promise.resolve()
| } | ||
|
|
||
| return blockToMerge.data | ||
| .then((blockToMergeInfo) => { |
There was a problem hiding this comment.
Точно ли промис вернется?
В data просто вызывается save(), может там стоит промисифицировать, чтобы наверняка?
| */ | ||
| getBlockIndex(block) { | ||
|
|
||
| for(let i = 0; i < this._blocks.length; i++) { |
src/components/modules/keyboard.js
Outdated
| this.Editor.Caret.setToBlock(this.Editor.BlockManager.currentBlock, 0, true); | ||
| this.Editor.Toolbar.close(); | ||
|
|
||
| }, 10); |
| .then((blockToMergeInfo) => { | ||
|
|
||
| targetBlock.mergeWith(blockToMergeInfo.data); | ||
| this.removeBlock(blockToMergeIndex); |
There was a problem hiding this comment.
можно оптимизировать дублирование remove с помощью then
| @@ -201,9 +248,15 @@ export default class BlockManager extends Module { | |||
| * @todo make object in accordance with Tool | |||
| */ | |||
| let data = { | |||
| text: wrapper.innerHTML, | |||
| text: wrapper.textContent.trim() === '' ? '' : wrapper.innerHTML, | |||
There was a problem hiding this comment.
на пустоту проверяем через this.currentBlock.isEmpty
| }; | ||
|
|
||
| if (this.currentBlock.isEmpty) { | ||
|
|
||
| this.currentBlock.pluginsContent.innerHTML = ''; |
There was a problem hiding this comment.
манипуляция с домом, дублирование проверки на пустоту
| * @param {Block} block | ||
| * @return {Number} | ||
| */ | ||
| getBlockIndex(block) { |
There was a problem hiding this comment.
такой метод уже есть в классе Block
src/components/modules/keyboard.js
Outdated
| * Handle backspace keypress on block | ||
| * @param event | ||
| */ | ||
| backSpacePressed(event) { |
src/components/modules/keyboard.js
Outdated
| backSpacePressed(event) { | ||
|
|
||
| let isFirstBlock = this.Editor.BlockManager.currentBlockIndex === 0, | ||
| canMergeBlocks = !this.Editor.BlockManager.currentBlock.hasMedia && this.Editor.Caret.isAtStart && !isFirstBlock; |
src/components/modules/keyboard.js
Outdated
| blockToMerge = this.Editor.BlockManager.currentBlock; | ||
|
|
||
|
|
||
| if (blockToMerge.name !== targetBlock.name || !this.Editor.BlockManager.currentBlock.mergeable) { |
There was a problem hiding this comment.
this.Editor.BlockManager.currentBlock -> targetBlock
|
|
||
| } else { | ||
|
|
||
| textNodeLength = lastNode.length; |
There was a problem hiding this comment.
У текстового узла тоже есть свойство textContent
src/components/dom.js
Outdated
| let child = atLast ? 'lastChild' : 'firstChild', | ||
| sibling = atLast ? 'previousSibling' : 'nextSibling'; | ||
|
|
||
| console.log('node is ', node); |
src/components/modules/keyboard.js
Outdated
| */ | ||
| backspacePressed(event) { | ||
|
|
||
| let isFirstBlock = this.Editor.BlockManager.currentBlockIndex === 0, |
There was a problem hiding this comment.
Если в начале функции деконструировать BlockManager, можно довольно сильно сократить код.
src/components/modules/keyboard.js
Outdated
| .then( () => { | ||
|
|
||
| // decrease current block index so that to know current actual | ||
| BM.currentBlockIndex--; |
There was a problem hiding this comment.
Думаю, это не должно здесь быть
No description provided.