feat #985 MultiAutoComplete: Public methods for highlight class #985

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Contributor

smadapathi commented Feb 21, 2014

This pull request adds public methods for adding, removing and getting highlighted suggestions for multiautocomplete widget

@flongo flongo self-assigned this Feb 21, 2014

@flongo flongo added this to the 1.4.17 milestone Feb 21, 2014

src/aria/widgets/form/Input.js
@@ -364,7 +364,6 @@ Aria.classDefinition({
// environment
cfg.directOnBlurValidation = aria.widgets.environment.WidgetSettings.getWidgetSettings().directOnBlurValidation;
}
-
@flongo

flongo Feb 21, 2014

Collaborator

If there is no real change, I think it is better to revert this file to its previous version.

+ removeHighlight : function (i) {
+ var suggestionContainer = this._textInputField.parentNode;
+ var typeUtil = aria.utils.Type;
+ if (!i) {
@flongo

flongo Feb 21, 2014

Collaborator

What if i is equal to 0?

+ }
+ if (typeUtil.isArray(i)) {
+ for (var k = 0; k < i.length; k++) {
+ var suggestionNode = suggestionContainer.children[i[k] - 1];
@flongo

flongo Feb 21, 2014

Collaborator

So it is 1-based, right? I am more in favour of a 0-based selection. Whatever you decide to keep, can you please add this information in the jsDoc?

+ }
+ }
+ } else {
+ var suggestionNode = suggestionContainer.children[i - 1];
@flongo

flongo Feb 21, 2014

Collaborator

You can also call this.removeHighlight([i]);

+ * @public
+ */
+ addHighlight : function (i) {
+ var suggestionContainer = this._textInputField.parentNode;
@flongo

flongo Feb 21, 2014

Collaborator

The same comments as above apply also here. It is just important to be consistent with the indexing strategy.

Collaborator

jakub-g commented Feb 21, 2014

I'd just like to add that it'll be better to have more meaningful commit messages.

Public methods for adding, removing and getting highlighted suggestions

The commit messages should be meaningful on the repo level, here we know the commit is about highlighted suggestions somewhere. It should be written explicitly that it's about MultiAutoComplete (probably the commit message should start from MultiAutoComplete:)

@@ -55,6 +59,17 @@
{call background(skinClass.optionsBackgroundColor,skinClass.closeSpriteURL,"no-repeat -5px -25px")/}
}
+ .xMultiAutoComplete_${skinClassName}_options a:hover{
@flongo

flongo Feb 21, 2014

Collaborator

If the same order was kept in the close crosses sequence (namely the :hover one on top) there would be no need to define this rule. Also, why is the extension of this image different with respect to the other one?

+ {call background(skinClass.optionsHighlightBackgroundColor,skinClass.closeHighlightSpriteURL,"no-repeat -5px -5px")/}
+ }
+ .highlight a:hover{
+ background-position: -5px -25px;
@flongo

flongo Feb 21, 2014

Collaborator

formatting issue here

@@ -45,7 +45,11 @@
border-radius:3px;
margin:2px;
}
-
+ .highlight{
@flongo

flongo Feb 21, 2014

Collaborator

I think that just having .highlight in the selector is a little error-prone as the name is generic. There might be interference. I would rather have .xMultiAutoComplete_${skinClassName}_options.highlight for better scoping.

@@ -29,6 +29,7 @@ Aria.classDefinition({
this.addTests("test.aria.widgets.form.autocomplete.multiautocomplete.test7.MultiAutoError");
this.addTests("test.aria.widgets.form.autocomplete.multiautocomplete.test8.MultiAutoMaxOptions");
this.addTests("test.aria.widgets.form.autocomplete.multiautocomplete.test9.MultiAutoBackSpace");
+ this.addTests("test.aria.widgets.form.autocomplete.multiautocomplete.test10.MultiAutoHighlightTest");
@flongo

flongo Feb 21, 2014

Collaborator

Is it possible to rename the test folder so that it is not called test10, but has a more specific value. The problem here is that other people working on a MultiAutoComplete issue are using the same folder for their test (I have already seen one).

+ this.checkSelectedItems(4);
+
+ widgetInstance.addHighlight([1, 3, 4]);
+ this.assertEquals([1, 3, 4].join(','), widgetInstance.getHighlight().join(','), "Highlighted suggestions are not proper");
@flongo

flongo Feb 21, 2014

Collaborator

You can also use this.assertJsonEquals instead of having to join the results.

smadapathi pushed a commit to smadapathi/ariatemplates that referenced this pull request Feb 24, 2014

smadapathi pushed a commit to smadapathi/ariatemplates that referenced this pull request Feb 24, 2014

smadapathi pushed a commit to smadapathi/ariatemplates that referenced this pull request Feb 24, 2014

@jakub-g jakub-g assigned jakub-g and unassigned flongo Feb 24, 2014

+ },
+ /**
+ * To remove the highlight class from the suggestion(s)
+ * @param {MultiTypes} i It can be an array of indices of suggestions or an index of suggestion. If nothing is
@jakub-g

jakub-g Feb 24, 2014

Collaborator

Instead of {MultiTypes} I'd put {Array|Integer}. I'd also rename the variable from i to indices.
Same applies to addHighlight

+ },
+ /**
+ * To remove class from DomElement
+ * @param {aria.utils.HTML} suggestionNode
@jakub-g

jakub-g Feb 24, 2014

Collaborator

I think it should be HTMLElement instead of aria.utils.HTML, right?
Same applies to addClass

Collaborator

jakub-g commented Feb 24, 2014

Please update the commit message (not only the pull request title) to include MultiAutoComplete keyword in it.

smadapathi pushed a commit to smadapathi/ariatemplates that referenced this pull request Feb 24, 2014

smadapathi pushed a commit to smadapathi/ariatemplates that referenced this pull request Feb 24, 2014

+ suggestionNodeClassList.$dispose();
+ },
+ /**
+ * Returns an array of indices of suggestions which have highlight class.
@jakub-g

jakub-g Feb 24, 2014

Collaborator

Missing @return {Array} jsdoc annotation here.

Collaborator

jakub-g commented Feb 24, 2014

The pull request is out of sync with master, please rebase it after all the fixes are put in place.

smadapathi pushed a commit to smadapathi/ariatemplates that referenced this pull request Feb 25, 2014

smadapathi pushed a commit to smadapathi/ariatemplates that referenced this pull request Feb 25, 2014

carlo-mr added a commit to carlo-mr/ariatemplates that referenced this pull request Mar 3, 2014

divdavem added a commit to divdavem/ariatemplates that referenced this pull request Mar 6, 2014

feat #985 Adding missing properties in AriaSkinBeans
In commit d31b18d, some properties
were added to the skin, but were not added to AriaSkinBeans.
This commit adds these properties to AriaSkinBeans.

divdavem added a commit to divdavem/ariatemplates that referenced this pull request Mar 6, 2014

fix #1014 Adding MultiAutoComplete highlight properties in AriaSkinBeans
In commit d31b18d (corresponding to #985),
some properties were added to the skin, but were not added to AriaSkinBeans.
This commit adds these properties to AriaSkinBeans.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment