Problem/Motivation

Convert type selectors to be compatible with with jQuery native-API selector for better performance

Proposed resolution

Follow this replacement pattern:

:eq -> $("your-pure-css-selector").eq(index)
:even -> .filter(":even") 
:odd ->.filter(":odd")
:first -> .filter(":first") or .eq(0)
:gt(index) -> $("your-pure-css-selector").slice(index)
:last -> .filter(":last")
:lt(index) -> $("your-pure-css-selector").slice(0, index)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tarekdj’s picture

Status: Needs work » Needs review
FileSize
10.23 KB
nod_’s picture

Status: Needs review » Needs work

using :even and :odd do not take us much closer to not using sizzle. The changes should be:

:eq -> $("your-pure-css-selector").eq(index)
:gt(index) -> $("your-pure-css-selector").slice(index)
:lt(index) -> $("your-pure-css-selector").slice(0, index)

OK.

:even -> .filter(":even")
:odd ->.filter(":odd")

We can leave those alone, they'll get removed eventually.

:first -> .first()
:last -> .last()

so something like:


-    this.$textElement = this.$el.find('.field-item:first');
+    this.$textElement = this.$el.find('.field-item').first();

Also we don't touch files in the assets/vendor folder, it's not ours.

And you should really use JSHint, you have syntax errors that makes the file completely useless :) https://drupal.org/node/1972428

tarekdj’s picture

I admit that this patch is really sloppy. Shame on me! :)

tarekdj’s picture

Status: Needs work » Needs review
FileSize
11.27 KB
12.44 KB

I tryed to follow your recomendations. Definitely better!

droplet’s picture

  1. +++ b/core/misc/tabledrag.js
    @@ -310,18 +310,18 @@ Drupal.tableDrag.prototype.makeDraggable = function (item) {
    -  var $indentationLast = $item.find('td:first .indentation:last');
    +  var $indentationLast = $item.find('td .indentation').last();
    

    Missing :first

  2. +++ b/core/misc/vertical-tabs.js
    @@ -102,7 +102,7 @@ Drupal.verticalTab = function (settings) {
    +      $('.vertical-tabs-pane').find('input,button,textarea,select').filter(':visible:enabled').first().trigger('focus');
    

    can we add space after comma

droplet’s picture

FileSize
6.4 KB
13.75 KB

- Fixed mismatch selectors
- :first -> eq(0)
- :last -> eq(-1)
- dropped one :input fix, we fix it in another issue thread.

The last submitted patch, 6: interdiff.patch, failed testing.

javisr’s picture

Patch applies cleanly and fixes the issue. I have verified that there are no more pending selectors to search and replace.

javisr’s picture

Status: Needs review » Reviewed & tested by the community
nod_’s picture

FileSize
17.84 KB

This is a reroll because the indentation standard changed for JavaScript files. No need for commit credit because of this reroll.

jibran’s picture

+++ b/core/modules/views_ui/js/views-admin.js
@@ -818,7 +816,7 @@
-      function changeDefaultWidget(event) {
+      function changeDefaultWidget (event) {

@@ -830,6 +828,7 @@
+

These changes are not present in RTBC patch. This is not a functional change but I think we should revert it because it is not related.

catch’s picture

Issue tags: +Needs manual testing

There's no confirmation on here that someone's manually tested the js that gets touched.

catch’s picture

Status: Reviewed & tested by the community » Needs review
quietone’s picture

Having a go on this at DrupalSouth 2014

quietone’s picture

Table drag works a treat, thanks.

What should I be looking for when doing manual testing of this patch?

Status: Needs review » Needs work

The last submitted patch, 10: core-js-jquery-position-2154475-10.patch, failed testing.

tarekdj’s picture

Assigned: tarekdj » Unassigned
Poornima3’s picture

FileSize
9.46 KB

I have converted some of the position selectors to be compatible with with jQuery native-API selector

zaporylie’s picture

Issue tags: +Needs reroll

As nod_ said "...we don't touch files in the assets/vendor folder, it's not ours."

And, definitely, patch #10 needs re-roll + changes suggested in #11

neelam.chaudhary’s picture

Status: Needs work » Needs review
Issue tags: +SprintWeekend2015
FileSize
16.05 KB

Reroll of patch #10 and the changes suggested in #11.

manningpete’s picture

Issue tags: -Needs reroll

patch applies.

nod_’s picture

Status: Needs review » Needs work

Please drop all spacing changes from this patch, my mistake for introducing them in the first place.

We already fixed the spacing issues in another issue (also be sure to run eslint when you make a patch to be sure you conform to our JS coding standards).

neelam.chaudhary’s picture

Status: Needs work » Needs review
FileSize
12.34 KB

As mentioned dropping all the spacing changes and retaining type selectors changes. Please review

jamin_melville’s picture

I've updated the patch, files had been updated since patch created.

nod_’s picture

Issue tags: +Needs reroll

Totally my fault but needs reroll. Sorry.

nod_’s picture

Status: Needs review » Needs work
lanchez’s picture

Assigned: Unassigned » lanchez

Rerolling.

lanchez’s picture

Assigned: lanchez » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
11.7 KB

Rerolled.

Do we want to use :not selector or jQuery .not?

The case is:
var $firstTab = this.details.siblings('.vertical-tabs__pane').not('.vertical-tab--hidden').eq(0);
vs
var $firstTab = this.details.siblings('.vertical-tabs__pane:not(.vertical-tab--hidden)').eq(0);

I guess that :not will be faster with "proper" browsers.

nod_’s picture

Yep that's CSS3 stuff since it's a simple selector inside the :not.

So yes, please use :not()

( edit ) patch already does it, great :)

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Block admin works
Contextual works
Editor works
Tabledrag works (keyboard and mouse) (on display mode (with group))
Responsive table works
Text summary works
Tour works
Vertical tabs works
Views admin works.

All good, thanks!

The last submitted patch, 19: 8.0.x-Convert_position_selectors-2154475-19.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This is a non disruptive change to javascript - therefore permitted during the beta. Committed 22ef398 and pushed to 8.0.x. Thanks!

  • alexpott committed 22ef398 on 8.0.x
    Issue #2154475 by tarekdj, neelam.chaudhary, droplet, nod_, lanchez,...

Status: Fixed » Closed (fixed)

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

droplet’s picture

Ouch, broken some features: #2489826: tabledrag is broken