Caret module: initial #242

khaydarov · 2017-12-27T05:47:46Z

No description provided.

gohabereg · 2017-12-28T23:30:14Z

src/components/selection.js

+     */
+    constructor() {
+
+        this.instance = null;


Зачем это, если все методы статичны?

gohabereg · 2017-12-28T23:30:51Z

src/components/utils.js

+     * Returns basic nodetypes as contants
+     * @return {{TAG: number, TEXT: number, COMMENT: number, DOCUMENT_FRAGMENT: number}}
+     */
+    static get nodeTypes() {


Для этого вроде какая-то нативная штука появилась

$1.nodeType === Node.ELEMENT_NODE $1.nodeType === Node.TEXT_NODE

etc

neSpecc · 2017-12-28T22:57:12Z

src/components/dom.js

+     * @param {Node} node
+     * @return {*|boolean}
+     */
+    static isLeaf(node) {


Need more evident name

neSpecc · 2017-12-28T22:58:43Z

src/components/dom.js

+    /**
+     * checks node if it is doesn't have child node
+     * @param {Node} node
+     * @return {*|boolean}


Why return type is any? Seems like it always returns boolean

neSpecc · 2017-12-28T22:59:56Z

src/components/dom.js

+    }
+
+    /**
+     * checks node if it is doesn't have child node


... any child nodes?

neSpecc · 2017-12-28T23:01:00Z

src/components/dom.js

@@ -35,6 +35,18 @@ export default class Dom {

    }

+    /**
+     * Creates text Node with content


Creates Text Node with the passed content

neSpecc · 2017-12-28T23:01:42Z

src/components/dom.js

@@ -86,6 +98,55 @@ export default class Dom {

    }

+    /**
+     * Search for deepest node


what does it means deepest?

neSpecc · 2017-12-28T23:17:34Z

src/components/dom.js

+     *
+     * @return {Boolean} true if it is empty
+     */
+    static checkNodeEmpty(node) {


isEmptyNode

neSpecc · 2017-12-28T23:21:45Z

src/components/dom.js

+
+            nodeText = node.value;
+
+            if ( nodeText.trim() ) {


You can move this block under the

if ( this.isElement(node) && this.isNativeInput(node) ) { } else { }

and remove similar one from the second part

And replace it with

return nodeText.trim().length === 0;

neSpecc · 2017-12-28T23:26:41Z

src/components/dom.js

+    }
+
+    /**
+     * breadth-first search


It doesn't looks like anyone can understand this.

neSpecc · 2017-12-28T23:29:32Z

src/components/dom.js

+
+        if (!node) {
+
+            return false;


undefined is not empty Node? Maybe you should throw an Error there? or add a console.assert

neSpecc · 2017-12-28T23:30:30Z

src/components/dom.js

+    static isEmpty(node) {
+
+        let treeWalker = [],
+            stack = [];


what stack it is?

neSpecc · 2017-12-28T23:36:09Z

src/components/utils.js

+     * Returns basic nodetypes as contants
+     * @return {{TAG: number, TEXT: number, COMMENT: number, DOCUMENT_FRAGMENT: number}}
+     */
+    static get nodeTypes() {


$1.nodeType === Node.ELEMENT_NODE $1.nodeType === Node.TEXT_NODE

etc

neSpecc · 2017-12-28T23:38:16Z

src/components/modules/blockManager.js

+                break;
+            case _.keyCodes.DOWN:
+            case _.keyCodes.RIGHT:
+                this.blockRightOrDownArrowPressed();


goToNextBlock

neSpecc · 2017-12-28T23:38:31Z

src/components/modules/blockManager.js

+                break;
+            case _.keyCodes.UP:
+            case _.keyCodes.LEFT:
+                this.blockLeftOrUpArrowPressed();


goToPrevoiusBlock

neSpecc · 2017-12-28T23:39:35Z

src/components/modules/blockManager.js

+     */
+    blockRightOrDownArrowPressed() {
+
+        let currentBlock = this.currentBlock,


redundant variable

neSpecc · 2017-12-28T23:40:35Z

src/components/modules/blockManager.js

+            lastTextNode = $.getDeepestTextNode(currentBlock.pluginsContent, true),
+            textNodeLength = lastTextNode.length;
+
+        if (Selection.getSelectionAnchorNode() !== lastTextNode) {


Selection.getAnchorNode

neSpecc · 2017-12-28T23:43:32Z

src/components/modules/blockManager.js

@@ -128,6 +239,62 @@ export default class BlockManager extends Module {

    }

+    /**
+     *
+     * @return {*}


neSpecc · 2017-12-28T23:43:40Z

src/components/modules/blockManager.js

@@ -128,6 +239,62 @@ export default class BlockManager extends Module {

    }

+    /**
+     *


Description is missed

neSpecc · 2017-12-28T23:44:20Z

src/components/modules/blockManager.js

+    }
+
+    /**
+     *


Desc is missed

neSpecc · 2017-12-28T23:44:32Z

src/components/modules/blockManager.js

+     * Returns next Block instance
+     * @return {*}
+     */
+    getNextBlock() {


It can be implemented via getter nextBlock

neSpecc · 2017-12-28T23:44:43Z

src/components/modules/blockManager.js

+    /**
+     * Returns previous Block instance
+     */
+    getPreviousBlock() {


It can be implemented via getter previousBlock

neSpecc · 2017-12-29T22:01:49Z

docs/caret.md

@@ -0,0 +1 @@
+# CodeX Editor Caret Module


Remove or fill this document

neSpecc · 2017-12-29T22:06:08Z

src/components/modules/blockManager.js

+
+        }
+
+        if (Selection.getSelectionAnchorOffset() === textNodeLength) {


Redundant «Selection» word

neSpecc · 2017-12-29T22:06:18Z

src/components/modules/blockManager.js

+        let lastTextNode = $.getDeepestNode(this.currentBlock.pluginsContent, true),
+            textNodeLength = lastTextNode.length;
+
+        if (Selection.getSelectionAnchorNode() !== lastTextNode) {


Redundant «Selection» word.

neSpecc · 2017-12-29T22:06:53Z

src/components/modules/blockManager.js

+    /**
+     * @todo Refactor method when code above will be moved to the keydown module
+     */
+    navigatePrevious() {


Description is missed

neSpecc · 2017-12-29T22:06:59Z

src/components/modules/blockManager.js

+    /**
+     * @todo Refactor method when code above will be moved to the keydown module
+     */
+    navigateNext() {


Description is missed

neSpecc · 2017-12-29T22:13:10Z

src/components/modules/ui.js

@@ -224,6 +224,14 @@ export default class UI extends Module {

        let clickedNode = event.target;

+        console.log('click', clickedNode);
+        if ( clickedNode.classList.contains(this.CSS.editorZone) ) {


It won't work with clicks on inner-elements. What behaviour do you expect?

And it is already working with setToTheLastBlock. See below

neSpecc · 2017-12-29T22:14:14Z

src/components/modules/ui.js

@@ -352,6 +360,26 @@ export default class UI extends Module {

    }

+    clickedOnRedactorZone(event) {


docs are missed

And its duplicates https://github.com/codex-team/codex.editor/pull/242/files#diff-cbb94d819d5a907ec02909dbe8b598b1R238

neSpecc · 2017-12-29T22:17:03Z

src/components/selection.js

+/**
+ * Working with selection
+ */
+export default class Selection {


Bad name, that conflicts with window.Selection

neSpecc · 2017-12-29T22:18:11Z

src/components/selection.js

+
+    }
+
+    static getSelection() {


redundant Selection-word all method's names.

neSpecc · 2017-12-29T22:18:21Z

src/components/selection.js

+
+    }
+
+    static getSelection() {


docs are missed

neSpecc · 2018-01-03T13:28:52Z

src/components/modules/caret.js

         */
+        if ($.isEmpty(pluginsContent)) {


лучше оставить геттер isEmpty у Block. Там проверяется не только пустота контента. Можешь принести туда свой $.isEmpty вместо this._html.textContent.trim().length === 0

gohabereg · 2018-01-04T16:03:26Z

src/components/modules/caret.js


        }

+        _.delay( () => this.set(nodeToSet, offset), 20)();


Зачем тут это?

Caret module: initial

c2acc25

khaydarov requested review from gohabereg, neSpecc and talyguryn as code owners December 27, 2017 05:47

khaydarov added 4 commits December 28, 2017 21:41

improvements

cfef2bb

moving caret initial

2e6b107

small improvements

ef6f2d5

last changes, added docs

0ed9238

gohabereg requested changes Dec 28, 2017

View reviewed changes

neSpecc requested changes Dec 28, 2017

View reviewed changes

khaydarov added 3 commits December 29, 2017 03:08

requested changes

c7a5be8

implement getters instead of functions in block cursors

8adc54c

last requested changes

4575a24

neSpecc requested changes Dec 29, 2017

View reviewed changes

khaydarov added 2 commits December 30, 2017 18:15

caret module docs and last improvements

cf4a126

update docs

8f4b7e5

neSpecc requested changes Jan 3, 2018

View reviewed changes

khaydarov added 2 commits January 4, 2018 01:13

upgrade request

6c75a45

update docs

6d83636

gohabereg reviewed Jan 4, 2018

View reviewed changes

src/components/modules/caret.js Outdated

}

_.delay( () => this.set(nodeToSet, offset), 20)();

Copy link

Member

gohabereg Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Зачем тут это?

upd

cdf2ca7

neSpecc approved these changes Jan 4, 2018

View reviewed changes

gohabereg approved these changes Jan 4, 2018

View reviewed changes

todo on delays

6db88c3

khaydarov merged commit 6fd28f3 into rewriting-version2.0 Jan 4, 2018

khaydarov deleted the module-caret branch January 4, 2018 20:29

@@ @@ -128,6 +239,62 @@ export default class BlockManager extends Module { @@
                   }
+                  /**
+                   *


		}

		if (Selection.getSelectionAnchorOffset() === textNodeLength) {

		@@ -352,6 +360,26 @@ export default class UI extends Module {

		}

		clickedOnRedactorZone(event) {

Caret module: initial #242

Caret module: initial #242

Conversation

khaydarov commented Dec 27, 2017

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

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