As part of #1574470 [Meta] selectors clean-up

CommentFileSizeAuthor
#21 interdiff.txt1.86 KBnod_
#21 core-js-ajax-selector-cleanup-1751320-21.patch5.06 KBnod_
#20 selector-cleanup-ajax-js-1751320-17.patch4.49 KBmadhavvyas
#18 selector-cleanup-ajax-js-1751320-17.patch2.76 KBmadhavvyas
#15 selector-cleanup-ajax-js-1751320-15.patch2.76 KBmadhavvyas
#11 selector-cleanup-ajax-js-1751320-11.patch5.74 KBrteijeiro
#5 selectors_clean_up-1751320-5.patch6.65 KBannikaC
#1 selectors_clean_up-1751320-1.patch12.05 KBsxnc
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sxnc’s picture

sxnc CreditAttribution: sxnc commented
Status: Active » Needs review
FileSize
selectors_clean_up-1751320-1.patch12.05 KB

I tried having a go at this to cleanup the selectors, somebody definitely needs to take a look at it though :)

Status: Needs review » Needs work
Issue tags: -JavaScript, -JavaScript clean-up

The last submitted patch, selectors_clean_up-1751320-1.patch, failed testing.

sxnc’s picture

sxnc CreditAttribution: sxnc commented
Status: Needs work » Needs review

#1: selectors_clean_up-1751320-1.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +JavaScript, +JavaScript clean-up

The last submitted patch, selectors_clean_up-1751320-1.patch, failed testing.

annikaC’s picture

annikaC CreditAttribution: annikaC commented
FileSize
selectors_clean_up-1751320-5.patch6.65 KB

Rerolled for latest dev

annikaC’s picture

annikaC CreditAttribution: annikaC commented
Status: Needs work » Needs review
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Needs work

um what about the search.api.php and search.module changes? shouldn't be in the patch.

There was a change from .attr('href') to .attr('id') it doesn't look like this will work, do you have a page where we can make sure it still works as expected?

annikaC’s picture

annikaC CreditAttribution: annikaC commented

Ah this was just a straight re-roll minus settings.php(???), so more of a statement of intent to look at it further! Will remove those changes as wasn't sure what the intention of editing search.api.php and search.module was with the original one.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Oh I see, didn't catch the changes in the previous patch. At the time there was a i18n sprint going on in the same room, there seems to have been some problems during patch generation :)

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia commented
Issue summary: View changes
Parent issue: » #1574470: Selectors clean-up
rteijeiro’s picture

rteijeiro CreditAttribution: rteijeiro commented
Assigned: sxnc » Unassigned
Status: Needs work » Needs review
FileSize
selector-cleanup-ajax-js-1751320-11.patch5.74 KB

Re-rolled and fixed a few issues more.

droplet’s picture

droplet CreditAttribution: droplet commented
Priority: Normal » Minor
Status: Needs review » Needs work
Issue tags: +Needs reroll
madhavvyas’s picture

madhavvyas CreditAttribution: madhavvyas as a volunteer and at CIGNEX commented
Assigned: Unassigned » madhavvyas
madhavvyas’s picture

madhavvyas CreditAttribution: madhavvyas as a volunteer and at CIGNEX commented
Issue tags: +drupalconasia2016
madhavvyas’s picture

madhavvyas CreditAttribution: madhavvyas as a volunteer and at CIGNEX commented
FileSize
selector-cleanup-ajax-js-1751320-15.patch2.76 KB

Patch rerolled

madhavvyas’s picture

madhavvyas CreditAttribution: madhavvyas as a volunteer and at CIGNEX commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs review » Needs work
Issue tags: +Needs reroll, +Needs manual testing 
+++ b/core/misc/ajax.js
@@ -923,26 +924,26 @@
+        $new_content.hide();

Missing update on `var new_content = new_content_wrapped.contents();`

madhavvyas’s picture

madhavvyas CreditAttribution: madhavvyas as a volunteer and at CIGNEX commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
selector-cleanup-ajax-js-1751320-17.patch2.76 KB

Corrected Patch Re rolled

madhavvyas’s picture

madhavvyas CreditAttribution: madhavvyas as a volunteer and at CIGNEX commented
Status: Needs review » Needs work
Issue tags: +Needs reroll
madhavvyas’s picture

madhavvyas CreditAttribution: madhavvyas as a volunteer commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
selector-cleanup-ajax-js-1751320-17.patch4.49 KB

Finally finally rerolled....

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
FileSize
core-js-ajax-selector-cleanup-1751320-21.patch5.06 KB
interdiff.txt1.86 KB

Hey! glad to see you here Madhav :)

Patch was missing a new_content replacement and removed a }. Reroll, added some cache for an ajax command too.

andriyun’s picture

andriyun CreditAttribution: andriyun at Skilld, Drupal Ukraine Community commented

Look great for me
+1 to RTBC

druprad’s picture

druprad
Primary language English
CreditAttribution: druprad as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Perform manual testing on 8.0.4-dev. Looks good to me.
+RTBC

madhavvyas’s picture

madhavvyas CreditAttribution: madhavvyas as a volunteer commented

Hey! Theo, thanks for updating my patch!!

  • catch committed ec829de on 8.1.x 
    Issue #1751320 by madhavvyas, nod_, sxnc, annikaC, rteijeiro: Selectors...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x, thanks!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.0.x-dev » 8.1.x-dev

Status: Fixed » Closed (fixed)

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