Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

feat #914 MultiAutoComplete Widget with more changes #915

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

smadapathi commented Jan 6, 2014

This pull request allows the following features

  1. User can add suggestion on "Enter" key press
  2. Suggestion can be deleted on "Backspace" key
  3. On "Tab" key, focus goes to input back again
  4. Match at word boundaries
  5. Ellipsis for the long suggestion
    and some minor fixes.

@ghost ghost assigned divdavem Jan 10, 2014

@divdavem divdavem commented on an outdated diff Jan 10, 2014

...ia/widgets/controllers/MultiAutoCompleteController.js
@@ -116,7 +120,9 @@
addedValue = text;
}
this._suggestionToBeAdded = addedValue;
- if (this.selectedSuggestions.length > 0 && !init) {
+ if (this.maxOptions && this.selectedSuggestions.length < this.maxOptions && !init) {
+ addedValue = this._checkValidSuggestion(addedValue);
+ } else {
addedValue = this._checkValidSuggestion(addedValue);
}
@divdavem

divdavem Jan 10, 2014

Member

When you write code, please simplify it as much as possible...
If you do the same thing both when a condition is true and when it is false, you probably don't need the condition. Instead of:

if (a) {
   b();
} else {
   b();
}

Please use simply:

b();

(which is strictly equivalent only if evaluating condition a has no side effect, which is the case here).

@divdavem divdavem commented on an outdated diff Jan 21, 2014

src/aria/resources/handlers/LCRangeResourceHandler.js
@@ -89,21 +91,34 @@
codeSuggestions.push(suggestion.original);
suggestion.original.exactMatch = false;
} else {
- if (suggestion.label.substring(0, textEntryLength) === textEntry) {
+ var boundaries = multiWord ? suggestion.wordBoundaries : [0];
+ for (var j = 0, len = boundaries.length, boundary; j < len; j++) {
+ boundary = boundaries[j];
+ if (suggestion.label.substring(boundary, boundary + textEntryLength) !== textEntry) {
+ continue;
+ }
+
@divdavem

divdavem Jan 21, 2014

Member

All this code is very similar to (and probably copy-pasted from) the LCResourcesHandler

Instead of copy-pasting, please consider sharing code when it makes sense.

An LCRangeResourcesHandler instance is basically doing the same job as an LCResourcesHandler instance except that it is also able to support ranges. So, in the code of the LCRangeResourcesHandler, you should only find code to manage ranges, and everything else should be delegated somehow to the parent.

In the previous pull request about the multi-autocomplete (see here), I already suggested you to call the parent getSuggestions method from this method.
Later, you were asked to support something the parent getSuggestions method already supported.
If you had followed my advice, support for multi-word matching would have been present right from the beginning in LCRangeResourcesHandler.
Sharing code allows to automatically benefit from features and bug fixes implemented in the common code.

So, basically, here, the whole prototype of LCRangeResourcesHandler could be as simple as the following code (perhaps this code would have to be adapted a bit, I have written it here only for you to understand the idea):

        $prototype : {

            rangePattern : /^[a-z]{1}\d+-\d+/,

            getSuggestions : function (textEntry, callback) {
                if (!typesUtil.isString(textEntry) || textEntry.length < this.threshold) {
                    this.$callback(callback, null);
                    return;
                }
                textEntry = stringUtil.stripAccents(textEntry).toLowerCase();
                if (this.allowRangeValues && this.rangePattern.test(textEntry)) {
                    var firstLetter = textEntry.charAt(0);
                    var rangeV = textEntry.substring(1).split("-");
                    var results = {
                        suggestions : [],
                        multipleValues : true
                    };
                    for (var k = rangeV[0]; k <= rangeV[1]; k++) {
                        var textEntry = firstLetter + k;
                        this.$LCResourcesHandler.getSuggestions.call(this, textEntry, {
                            fn : this.__appendSuggestions,
                            scope : this,
                            args : results
                        });
                    }
                    this.$callback(callback, results);
                } else {
                    this.$LCResourcesHandler.getSuggestions.call(this, textEntry, callback);
                }
            },

            __appendSuggestions : function (suggestions, results) {
                if (suggestions) {
                    results.suggestions = results.suggestions.concat(suggestions);
                }
            }

        }

@divdavem divdavem commented on an outdated diff Jan 21, 2014

src/aria/widgets/form/MultiAutoComplete.js
this._editMultiselectValue(element, event);
}
}
},
/**
+ * Handling blur event
+ * @param {aria.utils.Event} event
+ * @protected
+ */
+ _dom_onblur : function (event) {
+ var inputField = this.getTextInputField();
+ if (inputField.parentNode.lastChild.nodeName !== "INPUT" && inputField.value === "") {
@divdavem

divdavem Jan 21, 2014

Member

Instead of this complex condition, you could use the nextSibling property to know whether the input field is the last child of its parent. So the condition would become:

if (inputField.nextSibling != null && inputField.value === "") {

@divdavem divdavem commented on an outdated diff Jan 22, 2014

src/aria/widgets/form/MultiAutoComplete.js
+ }
+ this.$TextInput._dom_onblur.call(this, event);
+ },
+ /**
+ * Make the inputfield as last child of widget
+ * @protected
+ */
+ _makeInputFieldLastChild : function () {
+ var domUtil = aria.utils.Dom;
+ if (this._frame.getChild(0).lastChild !== this._textInputField) {
+ domUtil.insertAdjacentHTML(this._frame.getChild(0).lastChild, "afterEnd", "<span></span>");
+ domUtil.replaceDomElement(this._frame.getChild(0).lastChild, this._textInputField);
+ this._textInputField.style.width = "0px";
+ this.__resizeInput();
+ this.setHelpText(false);
+ this._textInputField.focus();
@divdavem

divdavem Jan 22, 2014

Member

Note that this method is called on blur. Is it really a good idea to always move the focus back to the text field?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment