4
\$\begingroup\$

Introduction:

I am working on a javascript plugin AutoSuggest, My aim is to create an IDE like autocomplete experience in web applications using JavaScript.

One of the features I have implemented is focusing a particular portion of inserted snippet after insertion. This is how I am asking input for focus.

focusText: [StartIndex, EndIndex]

StartIndex and EndIndex of the content that should be in focus after inserting the text. As of now these indexes should be calculated as (0 - numberOfCharactersFromEnd)

focusHtml: [StartIndex, EndIndex]

StartIndex and EndIndex of the content that should be in focus after inserting the HTML. As of now these indexes should be calculated as (0 - numberOfCharactersFromEnd). This should not include the characters from HTML tags

Example:

{
    value: 'script',
    insertText: '<script type="text/javascript" src="path/to/jsfile"><\/script>',
    focusText: [-25, -11],
    insertHtml: `&lt;<span style="color: red">script</span>&nbsp;<span style="color:green">type</span>=<span style="color:darksalmon">"text/javascript"</span>&nbsp;<span style="color:green">src</span>=<span style="color:darksalmon">"path/to/jsfile"</span>&gt;&lt;\/<span style="color: red">script</span>&gt;`,
    focusHtml: [-25, -11],
}

// Should focus `path/to/jsfile` after inserting the snippet

insertText and focusText are for inserting snippet as text.

insertHtml and focusHtml are for inserting snippet as html elements.

And I am evaluating the focus value as follows:

/* For inserting as text:
 * textNode is the text node in which the snippet has been inserted
 * selection is the range object of current selection.
 * cursorPosition is after the last character in inserted snippet
 * */

selection.setStart(textNode, cursorPosition + focusText[0]);
selection.setEnd(textNode, cursorPosition + focusText[1]);
/* For inserting as HTML:
 * textNode is the text node after which the new html element(s) will be inserted
 * selection is the range object of current selection.
 * insertHtmlAfter returns an Array of newly inserted nodes
 * */
const nodes = insertHtmlAfter(textNode, suggestion.insertHtml);
const focus = nodes.length ? suggestion.focusHtml : [0, 0];
function setSelection(focus, nodes, method) {
    let lastNode, lastFocus = focus;
    if (lastFocus !== 0) {
        do {
            lastNode = nodes.pop();
            lastFocus += lastNode.textContent.length;
        } while(nodes.length && lastFocus < 0);

        if (!lastNode) {
            throw new TypeError(`AutoSuggest: Invalid value provided for Suggestion.focusHtml`);
        };
    }

    if (lastFocus === 0) {
        selection[method + 'After'](nodes[nodes.length - 1] || textNode);
    } else {
        if (lastNode.nodeType === lastNode.TEXT_NODE) {
            selection[method](lastNode, lastFocus);
        } else {
            setSelection(
                lastFocus - lastNode.textContent.length,
                Array.from(lastNode.childNodes),
                method
            );
        }
    }
};

setSelection(focus[1], [...nodes], 'setEnd');
setSelection(focus[0], [...nodes], 'setStart');

Problem:

  1. Asking for focus values as negative numbers might be confusing for the users, but it simplifies the logic for focusText, so is there any better way that is both user friendly and requires simpler logic.

  2. I think the logic for setting the focus for inserting as HTML is a bit over-complicated, is there any better way to achieve this?

\$\endgroup\$
  • \$\begingroup\$ You stated: "it simplifies the logic for focusText" that makes it sound like focusText is a function/method but after searching the repository that doesn't appear to be the case... what logic is simplified by having negative numbers? Please edit your post to include any logic you want reviewed... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Apr 3 at 19:45
  • \$\begingroup\$ I have included the logic for focusText in the question, the variables are a little different in original repo, you can find it in setValue method, line number 214 \$\endgroup\$ – AvcS Apr 5 at 5:37
  • \$\begingroup\$ I have posted both the snippets I want reviewed and Also if you can suggest better names for those params, please do. I am also little confused about how to name them :D \$\endgroup\$ – AvcS Apr 5 at 5:43
2
+25
\$\begingroup\$

I do feel that negative numbers seems a bit backwards compared to most other libraries I have used. I am not sure what all would be involved with reworking this code to utilize positive numbers but it seems it may be hefty. Though maybe you could accept positive numbers and convert them to negatives by subtracting them from the length of the string.


I see one spot that may have a possible performance improvement- in the recursive call to setSelection():

setSelection(
    lastFocus - lastNode.textContent.length,
    Array.from(lastNode.childNodes),
    method
);

here Array.from() is called to copy the array of nodes from the childNodes collection from lastNode. The spread operator can also be used to put those child nodes into an array (from the node collection), allowing you to eliminate that extra function call.

setSelection(
    lastFocus - lastNode.textContent.length,
    [...lastNode.childNodes],
    method
);
\$\endgroup\$
  • \$\begingroup\$ Hi sam, the variable nodes is an actual array and it is reused, I am using spread operator to clone the array, so that original array stays intact. lastNode.childNodes is not an array, so I have to convert it into an array but I don't need to clone it as it is not reused. \$\endgroup\$ – AvcS Apr 8 at 5:54
  • \$\begingroup\$ Why negative numbers? Conceptually whats happening is the cursor will be at the end of the line after inserting new text and I have to count back to place the cursor at the required point, so negative numbers make the logic simpler as all I have to do is add the number provided by user to current position and I will get the required position. That is simpler logic wise but I am not sure about user experience \$\endgroup\$ – AvcS Apr 8 at 6:00
  • \$\begingroup\$ I understand why you wanted to use negative numbers, and why you want to copy. Please see revised answer. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Apr 8 at 6:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.