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

CommentFileSizeAuthor
#10 core-js-selectors-book-1574620-10.patch491 bytesjibran
#4 core-js-selectors-book-4.patch516 bytesnod_
#2 core-js-selectors-book-1.patch522 bytesnod_
core-js-selectors-book.patch524 bytesnod_

Comments

nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue tags: +JavaScript clean-up

forgot tag

nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-js-selectors-book-1.patch522 bytes

wrong patch :/

droplet’s picture

droplet commented

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

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

nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-js-selectors-book-4.patch516 bytes

You're right. Less likely to break too.

droplet’s picture

droplet commented
Status: Needs review » Reviewed & tested by the community
dries’s picture

dries commented

Why are we making this change?

nod_’s picture

nod_
Primary language French
Location Lille
commented

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

nod_
Primary language French
Location Lille
commented

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

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

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

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
StatusFileSize
new core-js-selectors-book-1574620-10.patch491 bytes

patch for drupal-7

droplet’s picture

droplet commented
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

nod_
Primary language French
Location Lille
commented
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

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