Allow autoComplete to use a select as source of suggestions #709

adamliptrot-oc · 2016-11-16T16:59:44Z

No description provided.

feedmypixel

@adamliptrot-oc Good work and some nice backward compatibility guarding. Overall you have work in the factory which can be moved inside the autoComplete.js file. Also please update documentation here and here

feedmypixel · 2016-11-17T13:28:00Z

assets/javascripts/modules/autoCompleteFactory.js

-  var suggestionDisplayTemplate = function (title, value) {
-    return title + ' (+' + value + ')';
-  };
+  $('.js-choose-country-auto-complete').each(function(){


Is this to allow multiple AutoCompletes? or just a different way of running the code if the element exists? I would be wary of enabling multiples

It was yes, so we enforce one instance per page?

how are you going to handle multiple autoCompletes with different data? Not sure how wonderful a page with 2 autoCompletes would look! ;) I would call YAGNI on this

feedmypixel · 2016-11-17T14:27:29Z

assets/javascripts/modules/autoCompleteFactory.js

+      $countryCodeInput = $('#countryCode');
+    } else {
+      $countryCodeInput = $('[id="' + $(this).attr("data-suggestions") + '"]');    
+    }


so data-suggestions is used for validation here and the $countryCodeInput is used as the $targetInputElem described here

So with those explanations (data-suggestions validation needs documenting) I think you would agree it feels a bit mixed up whats going on here.

What you are wanting to say is target your select. This "target" element should be sent in via a new data-.

For backwards compatibility you need to keep the $countryCodeInput work (as you have done) but I would move this check inside the autoComplete.js file here Then you can check to see if it is an element or a string and do the appropriate.

I wanted to allow $countryCodeInput to be definable from the view as it is currently hard-coded in the factory. If the ID is already defined in the view (or the list is not one of countries) we need to allow this (using a defined ID also restricts instance use).

Agree that if data-suggestions is used elsewhere for validation I need to separate out my added functionality to make it clearer - didn't realise that integration was there. I had done this originally but decided to re-use the data-suggestions attribute as it seemed un-used. Will update.

I wanted to allow $countryCodeInput to be definable from the view as it is currently hard-coded in the factory. If the ID is already defined in the view (or the list is not one of countries) we need to allow this (using a defined ID also restricts instance use).

Agreed. So I think we are saying the same thing here. Send in the $countryCodeInput via a data attribute. The #countryCode just needs to be kept for backwards compatibility.
I see a future bit of work to remove the #countryCode selector from the Js and move it into the data- attribute in the view for services who have not refactored to the data- way.

As said above for this check it doesn't feel right to have it in the factory, this can also go into the autoComplete.js

feedmypixel · 2016-11-17T14:31:50Z

assets/javascripts/modules/autoCompleteFactory.js

+      return title + ' (+' + value + ')';
+    };
+
+    var suggestions;


Please place vars at the top of a function due to hoisting

feedmypixel · 2016-11-17T14:45:11Z

assets/javascripts/modules/autoCompleteFactory.js

+    };
+
+    var suggestions;
+    if($('select[id="' + $(this).attr("data-suggestions") + '"]').length > 0){    


so you are building suggestions from the target element. This also feels like it shouldn't be in the factory it should be internal in the autoComplete.js file.
So I would make the second argument suggestions accept either a string or an object. If a string get that element, if an object do what you currently do. I would also abstract the work of greating suggestion into a high level function for self documentation createSuggestionsFromSelect for instance

Sounds good

feedmypixel · 2016-11-17T14:47:05Z

assets/javascripts/modules/autoCompleteFactory.js

+    var suggestions;
+    if($('select[id="' + $(this).attr("data-suggestions") + '"]').length > 0){    
+      suggestions = [];
+      $('[id="' + $(this).attr("data-suggestions") + '"]').find('option').each(function(){


you can cache a few selector calls here into variables

feedmypixel · 2016-11-17T14:51:29Z

assets/javascripts/modules/autoComplete.js

@@ -18,7 +18,7 @@ Auto complete html markup:
           autocomplete="off"
           spellcheck="false"
           required
-           data-suggestions="countries"
+           data-suggestions="{{OPTIONAL: id of select input to use as suggestions source}}"


this data-suggestions attribute is used for validation purposes here (needs documenting)

adamliptrot-oc · 2016-11-21T16:58:10Z

Just updated the autoComplete, moving most of the config into the main file as it can deduce most of it from the element's attributes.

feedmypixel · 2016-11-21T17:20:52Z

@adamliptrot-oc Your PR has gone slightly off in an unexpected direction.

So we now have two ways of instantiating an autoComplete:
• There is the current way of instantiating an autoComplete and passing in the variables as shown for the country example.
• Your way of sending in countries and $countryCodeInput via data- attributes.

I like the way of sending in these details via data- attributes rather than specifying them in autoCompleteFactory.js. So to be backwardly compatible your PR needs to instantiate an autoComplete that picks these arguments via data-attributes but also caters for the current way of passing these data attributes in.
The countryCode instantiation in autoCompleteFactory.js can be left untouched.

Have a read through the comments I have added on the original PR and lets have a hangout to confirm the direction.

feedmypixel · 2016-11-21T17:26:23Z

assets/javascripts/modules/autoComplete.js

+      suggestions = countries;
+    } else {
+      // allow custom variable
+      suggestions = window[el.attr("data-suggestions")];


I would avoid muddying the waters here, data-suggestions is for validation only if you wish to have a way of sending in a global then create a new data- attribute and make this backwardly compatible with the suggestionsData argument

feedmypixel · 2016-11-21T17:27:32Z

assets/javascripts/modules/autoComplete.js

+    // need to populate the data-suggestions attribute from this for validation
+    // generate a global variable to hold the data and insert into data-suggestions attribute
+    var r = randomVariableName();
+    window[r] = suggestions;


I'm not sure why this needs to be exposed as a global?

As the validator expects a global. Agreed this could be changed but it is outside the scope of this PR and can be picked up in a separate one.

agreed, please capture this by raising an issue around this work

feedmypixel · 2016-11-21T17:34:59Z

assets/javascripts/modules/autoComplete.js

+    }
+  }
+  return target;
+};


I think my comment has been misunderstood. Keep the original instantiation and in the autoCompleteFactory.js and inside of autoComplete.js check if the argument is an element, if not then check the data-linked attribute

Then you can check to see if it is an element or a string and do the appropriate.

feedmypixel · 2016-11-25T09:45:58Z

assets/javascripts/modules/autoComplete.js

           data-suggestions="countries"
+           // or specify a select input ID to act as source of data:
+           data-linked="mySelectID"


data-linked doesn't means anything to me? data-select-elem ?

feedmypixel · 2016-11-25T09:46:33Z

assets/javascripts/modules/autoComplete.js

+/**
+  * Generate random variable name to ensure no conflicts
+*/
+var randomVariableName = function () {


we don't need this randomVariableName functionality right? just place it on window.suggestions and at the moment we don't support 2x autoCompletes on the same page. As discussed the placing this on window will be removed in future work

I'd prefer not to use anything which could possibly be a conflict in the global space, so I think this should stay until the additional future work to remove the global object is completed.

IMO guarding against a conflict with creating a random global variable is hiding the issue away from the developer. If the global is being overridden due to a variable of the same name already existing the code shouldn't abstract that away and guard for it, the developer should change the name of the global to fix the problem at the source.

feedmypixel · 2016-11-25T09:53:21Z

assets/javascripts/modules/autoComplete.js

+var getTargetInput = function (el) {
+  var target;
+  // input to post back a selection from the autocomplete
+  var dataPostTo = $('[id="' + el.attr("data-post-to") + '"]');


data-target-elem feels a little bit more descriptive when looking at it in the html. data-post-to feels a bit POST form'y ... thoughts?

feedmypixel · 2016-11-25T09:55:07Z

assets/javascripts/modules/autoCompleteFactory.js

@@ -2,18 +2,28 @@ require('jquery');
 var autoComplete = require('./autoComplete.js');

 var createAutoCompleteCountries = function () {


want to also add an issue around removing this deprecated work? and playing any html updates in service frontends?

feedmypixel · 2016-11-28T14:40:25Z

LGTM 👍 - before merge please squash your commits into sensible commits, there feels like a lot of W.I.P commits here

generated from commit 329c37b

feedmypixel reviewed Nov 17, 2016

View reviewed changes

feedmypixel reviewed Nov 21, 2016

View reviewed changes

feedmypixel reviewed Nov 25, 2016

View reviewed changes

Allow AutoComplete to use select as source of suggestions

a996347

adamliptrot force-pushed the patch-2 branch from 37b6b05 to a996347 Compare November 28, 2016 14:47

feedmypixel merged commit 329c37b into hmrc:master Nov 28, 2016

hmrc-web-operations pushed a commit that referenced this pull request Nov 28, 2016

publish: Merge pull request #709 from adamliptrot-oc/patch-2

3fc3a8e

generated from commit 329c37b

rpowis mentioned this pull request Dec 2, 2016

AutoComplete validation requires a global variable #716

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow autoComplete to use a select as source of suggestions #709

Allow autoComplete to use a select as source of suggestions #709

adamliptrot-oc commented Nov 16, 2016

feedmypixel left a comment

feedmypixel Nov 17, 2016

adamliptrot-oc Nov 17, 2016

feedmypixel Nov 17, 2016

feedmypixel Nov 17, 2016

adamliptrot-oc Nov 17, 2016 •

edited

Loading

feedmypixel Nov 17, 2016

feedmypixel Nov 17, 2016

feedmypixel Nov 17, 2016

adamliptrot-oc Nov 17, 2016

feedmypixel Nov 17, 2016

feedmypixel Nov 17, 2016

adamliptrot-oc commented Nov 21, 2016

feedmypixel commented Nov 21, 2016

feedmypixel Nov 21, 2016

feedmypixel Nov 21, 2016

adamliptrot-oc Nov 24, 2016

feedmypixel Nov 24, 2016 •

edited

Loading

adamliptrot-oc Nov 24, 2016

feedmypixel Nov 21, 2016

feedmypixel Nov 25, 2016

feedmypixel Nov 25, 2016

adamliptrot-oc Nov 25, 2016

feedmypixel Nov 28, 2016

feedmypixel Nov 25, 2016

feedmypixel Nov 25, 2016

adamliptrot-oc Nov 28, 2016

feedmypixel commented Nov 28, 2016 •

edited

Loading

		@@ -2,18 +2,28 @@ require('jquery');
		var autoComplete = require('./autoComplete.js');

		var createAutoCompleteCountries = function () {

Allow autoComplete to use a select as source of suggestions #709

Allow autoComplete to use a select as source of suggestions #709

Conversation

adamliptrot-oc commented Nov 16, 2016

feedmypixel left a 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

adamliptrot-oc Nov 17, 2016 • edited Loading

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

adamliptrot-oc commented Nov 21, 2016

feedmypixel commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feedmypixel Nov 24, 2016 • edited Loading

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

feedmypixel commented Nov 28, 2016 • edited Loading

adamliptrot-oc Nov 17, 2016 •

edited

Loading

feedmypixel Nov 24, 2016 •

edited

Loading

feedmypixel commented Nov 28, 2016 •

edited

Loading