Added a scope to the select search.
Changed the selector to match the select name instead of relying on a wrapper class'.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_’s picture

Issue tags: +JavaScript clean-up

forgot tag

nod_’s picture

FileSize
522 bytes

wrong patch :/

droplet’s picture

Above patch looks good. Considering for performance, I preferred use this class.

'#attributes' => array('class' => array('book-title-select')),

nod_’s picture

FileSize
516 bytes

You're right. Less likely to break too.

droplet’s picture

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

Why are we making this change?

nod_’s picture

The patch scope the search of the form input to the current vertical tab. Previously it would search from the whole document.
The previous selector was not very optimized: .some-class-name select will always be slower to match than .some-class-name.

I haven't run into bugs because of all that but it improves performance. I wouldn't call it micro-optimization since this is the DOM we're talking about.

nod_’s picture

benchmark backing it up: http://jsperf.com/drupal-book-selector At least 10x faster. In chrome it's actually 100x faster.

Changing to this types of selector will make using the native querySelector much easier. Added the native selector to the benchmark, it's between 2x (opera) and 390x (chrome!) faster.

Now this is just one file, we have tens of files with the same kind of things, it sure doesn't look like it but it's an important patch.

Dries’s picture

Version: 8.x-dev » 7.x-dev

Thanks for the clarifications. Committed to 8.x. Moving to 7.x.

jibran’s picture

patch for drupal-7

droplet’s picture

Status: Reviewed & tested by the community » Needs review

It may not safely backport to D7 directly since someone may change the class name. (minor version upgrade issue)

nod_’s picture

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

We don't have the manpower to backport this kind of patches. If this gets in 7, then all the other selectors patches should get in 7 and they are going to be more invasive than this one. Can't be sure it won't break contrib and manual testing doesn't scale.

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

Manuel Garcia’s picture

Issue summary: View changes
Parent issue: » #1574470: Selectors clean-up