Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Clean up every little things
- code style
- performance (replace attr, don't always do foreach in autocompleteSubmit)
- use strict operators
Comment | File | Size | Author |
---|---|---|---|
#19 | ie7.png | 13.1 KB | droplet |
#19 | ie7-problem.png | 3.57 KB | droplet |
#16 | autocomlelte.patch | 4.66 KB | droplet |
#13 | autocomplete.patch | 4.66 KB | droplet |
#10 | core-js-autocomplete-clean-up-1420798-10.patch | 4.68 KB | nod_ |
Comments
Comment #1
nod_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.Comment #2
droplet CreditAttribution: droplet commentedComment #3
nod_You're sure about
if (this.selected === false)
? Looks like you reversed the condition,$(false).removeClass('selected')
will not work.Comment #4
droplet CreditAttribution: droplet commentedWhoops. also added comments. Thanks.
Comment #5
nod_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.
Comment #7
nod_how stupid is making a patch aginst D7 here :þ
Comment #8
droplet CreditAttribution: droplet commentedLooks Good now.
Comment #9
Dries CreditAttribution: Dries commentedMissing space between "if" and "(".
Comment #10
nod_sorry about that
Comment #11
droplet CreditAttribution: droplet commentedback to RTBC
Comment #12
catchCommitted/pushed to 8.x.
Moving to 7.x for backport.
Comment #13
droplet CreditAttribution: droplet commentedidentical autocomplete.js in D8 & D7
Comment #14
droplet CreditAttribution: droplet commented#13: autocomplete.patch queued for re-testing.
Comment #16
droplet CreditAttribution: droplet commentedafter this patch, there is 2 differences between D7 & D8.
#1419968: Replace $('selector', domelement) with $(domelement).find('selector')
Comment #17
jcisio CreditAttribution: jcisio commentedJust a simple reroll from D8 patch.
Comment #18
webchickCan you clarify the browsers this has been tested with? in Drupal 7, we need to see testing in IE 6 & 7.
Comment #19
droplet CreditAttribution: droplet commentedTested 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
Comment #20
droplet CreditAttribution: droplet commentedComment #21
cosmicdreams CreditAttribution: cosmicdreams commentedOn my schedule for tomorrow night
Comment #22
droplet CreditAttribution: droplet commented#16: autocomlelte.patch queued for re-testing.
Comment #23
nod_We don't backport cleanup JS patches. No automated testing and not enough manpower.
Comment #24
droplet CreditAttribution: droplet commentedSadly 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.
Comment #25
nod_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.
Comment #26
sharmavarun CreditAttribution: sharmavarun commentedautocomplete_clean_up.patch queued for re-testing.
Comment #31
droplet CreditAttribution: droplet commented