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

Support from Acquia helps fund testing for Drupal Acquia logo

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
FileSize
4.61 KB
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
FileSize
4.68 KB

Whoops. also added comments. Thanks.

nod_’s picture

Status: Needs work » Needs review
FileSize
4.66 KB

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

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
FileSize
4.68 KB

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
FileSize
4.66 KB

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
FileSize
4.66 KB

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

FileSize
3.57 KB
13.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

  • catch committed 99438b5 on 8.3.x
    Issue #1420798 by nod_, droplet: Autocomplete.js clean up.
    
    

  • catch committed 99438b5 on 8.3.x
    Issue #1420798 by nod_, droplet: Autocomplete.js clean up.
    
    

  • catch committed 99438b5 on 8.4.x
    Issue #1420798 by nod_, droplet: Autocomplete.js clean up.
    
    

  • catch committed 99438b5 on 8.4.x
    Issue #1420798 by nod_, droplet: Autocomplete.js clean up.
    
    
droplet’s picture

Version: 7.x-dev » 8.0.x-dev
Issue summary: View changes
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.