Clean up every little things
- code style
- performance (replace attr, don't always do foreach in autocompleteSubmit)
- use strict operators

Files: 
CommentFileSizeAuthor
#19 ie7.png13.1 KBdroplet
#19 ie7-problem.png3.57 KBdroplet
#16 autocomlelte.patch4.66 KBdroplet
PASSED: [[SimpleTest]]: [MySQL] 39,227 pass(es).
[ View ]
#13 autocomplete.patch4.66 KBdroplet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch autocomplete.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#10 core-js-autocomplete-clean-up-1420798-10.patch4.68 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-autocomplete-clean-up-1420798-10.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#7 core-js-autocomplete-clean-up-1420798-7.patch4.68 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 34,011 pass(es).
[ View ]
#5 core-js-autocomplete-clean-up-1420798-5.patch4.66 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-autocomplete-clean-up-1420798-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#4 autocomplete_clean_up.patch4.68 KBdroplet
PASSED: [[SimpleTest]]: [MySQL] 33,352 pass(es).
[ View ]
#2 autocomplete_clean_up.patch4.61 KBdroplet
PASSED: [[SimpleTest]]: [MySQL] 33,353 pass(es).
[ View ]
autocomplete_clean_up.patch4.61 KBdroplet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch autocomplete_clean_up.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

nod_’s picture

Status:Needs review» Needs work

Mostly good,

  • $("#autocomplete") needs to be cached, and since it's an ID it doesn't make sense to keep .each,
  • typeof value === "undefined", it's comparing to a string.
droplet’s picture

Status:Needs work» Needs review
StatusFileSize
new4.61 KB
PASSED: [[SimpleTest]]: [MySQL] 33,353 pass(es).
[ View ]
nod_’s picture

Status:Needs review» Needs work

You're sure about if (this.selected === false)? Looks like you reversed the condition, $(false).removeClass('selected') will not work.

droplet’s picture

Status:Needs work» Needs review
StatusFileSize
new4.68 KB
PASSED: [[SimpleTest]]: [MySQL] 33,352 pass(es).
[ View ]

Whoops. also added comments. Thanks.

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new4.66 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-autocomplete-clean-up-1420798-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Looks good to me, but needed a reroll, please confirm it's still good droplet.

It's rtbc for me, check I didn't mess up your patch anyway :)

The patch includes #784670: autocomplete.js can iterate over functions in Array objects with it too.

Status:Needs review» Needs work

The last submitted patch, core-js-autocomplete-clean-up-1420798-5.patch, failed testing.

nod_’s picture

StatusFileSize
new4.68 KB
PASSED: [[SimpleTest]]: [MySQL] 34,011 pass(es).
[ View ]

how stupid is making a patch aginst D7 here :þ

droplet’s picture

Status:Needs review» Reviewed & tested by the community

Looks Good now.

Dries’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/misc/autocomplete.jsundefined
@@ -30,9 +30,11 @@ Drupal.behaviors.autocomplete = {
+  var $autocomplete = $('#autocomplete');
+  if($autocomplete.length !== 0) {

Missing space between "if" and "(".

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new4.68 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-autocomplete-clean-up-1420798-10.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

sorry about that

droplet’s picture

Status:Needs review» Reviewed & tested by the community

back to RTBC

catch’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed/pushed to 8.x.

Moving to 7.x for backport.

droplet’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new4.66 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch autocomplete.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

identical autocomplete.js in D8 & D7

droplet’s picture

#13: autocomplete.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+needs backport to D7, +JavaScript clean-up

The last submitted patch, autocomplete.patch, failed testing.

droplet’s picture

Status:Needs work» Needs review
Issue tags:+Novice
StatusFileSize
new4.66 KB
PASSED: [[SimpleTest]]: [MySQL] 39,227 pass(es).
[ View ]

after this patch, there is 2 differences between D7 & D8.
#1419968: Replace $('selector', domelement) with $(domelement).find('selector')

jcisio’s picture

Status:Needs review» Reviewed & tested by the community

Just a simple reroll from D8 patch.

webchick’s picture

Status:Reviewed & tested by the community» Needs review

Can you clarify the browsers this has been tested with? in Drupal 7, we need to see testing in IE 6 & 7.

droplet’s picture

StatusFileSize
new3.57 KB
new13.1 KB

Tested IE6 & 7.

But in IE7 has one problem. when you don't focus on the word (admin), It's no highlighting & not able to select it. (not a regression)

new IE7 bug: #1553464: Can't select autocomplete items in IE7

droplet’s picture

Issue tags:+Needs manual testing
cosmicdreams’s picture

On my schedule for tomorrow night

droplet’s picture

#16: autocomlelte.patch queued for re-testing.

nod_’s picture

Version:7.x-dev» 8.x-dev
Assigned:droplet» Unassigned
Status:Needs review» Fixed

We don't backport cleanup JS patches. No automated testing and not enough manpower.

droplet’s picture

Version:8.x-dev» 7.x-dev
Priority:Normal» Minor
Status:Fixed» Needs review

Sadly to see it but there have already have a Patch. and this is a code standard fix too. I set priority to minor. I could drop hasOwnProperty and strict operator from it.

nod_’s picture

Actually the hasownproperty would fix a bug linked in #5 so i don't think we need to drop anything from it. It just needs testing. I don't have the time ATM, I can't rtbc it and nobody picked it up since April.

By all mean if you have people to test it, get that in that'd be a good thing.

sharmavarun’s picture