Removed duplicated code.
id selectors shouldn't have any element and don't need to be scoped.

I haven't touched at the second behavior, it should be postponed until the tabledrag rewrite.

CommentFileSizeAuthor
#22 removed_unnecessary_programming_from_previous_patch-1574512-22.patch762 bytesMariaAlicia
#18 removed_unnecessary_programming_from_previous_patch-1574512-18.patch776 bytesMariaAlicia
#15 core-js-selectors-block-1574512-15.patch5.48 KBoresh
#1 core-js-selectors-block-1.patch3.36 KBnod_
core-js-selectors-block.patch3.37 KBnod_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
FileSize
core-js-selectors-block-1.patch3.36 KB

Use a CSS selector instead of traversing to get the label text

pounard’s picture

pounard
Primary language French
CreditAttribution: pounard commented

Is that normal you loose the context parameter of the attach() function?

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Yes because a scoped search for an ID does not make sense and it is not faster by any significant amount.

The other "context" variable is related to vertical tabs and is totally different from the attach parameter.

So yep, it's normal

droplet’s picture

droplet CreditAttribution: droplet commented

why introduce CONTEXT at beginning ? only for performance? Seems like 95%+ of time, "context = document" ?

loose context implicit could not changing the scope..
valid HTML page, ID is unique. But .classA #id, .classB #id are difference.

pounard’s picture

pounard
Primary language French
CreditAttribution: pounard commented

context is more than that, it also avoid potential duplicate calls when you attach back a result coming from AJAX request, or crafted DOM sections. It's cleaner architecturally since you always work in an encaspuled context.

But, in that specific case what I told has no sense and I agree with nod_, there is no need for context since it's working on ids, it just removes the unused variable from scope.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

We're not relying on context take care of the duplicate call issue, that's what we use .once() for. But yeah, typically it's cleaner.

You're raising an very interesting point droplet, we will need to be careful about a .classA #is and .classB #id scenario. Though it is not an issue in this case.

One of you care to test and RTBC, pretty please? :)

pounard’s picture

pounard
Primary language French
CreditAttribution: pounard commented
Status: Needs review » Reviewed & tested by the community

I didn't tested myself on my browser, but I dare to trust you about this one, the patch seems straightfoward enough :)

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Well for this one it's fine but beware of JS patches, they can bite :)

pounard’s picture

pounard
Primary language French
CreditAttribution: pounard commented

Of course, if there is someone else to test with IE which I can't do, working fine with FF 11 and Chromium 18

pounard’s picture

pounard
Primary language French
CreditAttribution: pounard commented

@nod_ Yes I ended up testing after 2 seconds I clicked the submit button.

pounard’s picture

pounard
Primary language French
CreditAttribution: pounard commented

Also working with Epiphany (now known as Web since Gnome 3.4) but not working in konqueror 4.8.3, but I'm not sure this is the JS for konqueror, since no summary at all anywhere seems to show up.

catch’s picture

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

Looks good to me, committed/pushed to 8.x.

Status: Fixed » Closed (fixed)

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

Kiphaas7’s picture

Kiphaas7 CreditAttribution: Kiphaas7 commented

Investigate if adding basic events and detach makes sense, as described in #1763812: [META] Provide complete attach/detach with basic events

looks like it could use a detach?

oresh’s picture

oresh CreditAttribution: oresh commented
Status: Closed (fixed) » Needs review
FileSize
core-js-selectors-block-1574512-15.patch5.48 KB

Small patch for lighter js and some performance clean up (remove var duplication and other).

droplet’s picture

droplet CreditAttribution: droplet commented 
+++ b/core/modules/block/block.jsundefined
@@ -26,9 +26,9 @@ Drupal.behaviors.blockSettingsSummary = {
-    $('#edit-visibility-node-type').drupalSetSummary(checkboxesSummary);
-    $('#edit-visibility-language').drupalSetSummary(checkboxesSummary);
-    $('#edit-visibility-role').drupalSetSummary(checkboxesSummary);
+    $('#edit-visibility-node-type, ¶
+      #edit-visibility-language, ¶
+      #edit-visibility-role').drupalSetSummary(checkboxesSummary);

To me it seems only these 3 lines is needed.

MariaAlicia’s picture

MariaAlicia CreditAttribution: MariaAlicia commented
Assigned: Unassigned » MariaAlicia
Issue summary: View changes

Assigned to myself so that I can work on it.

MariaAlicia’s picture

MariaAlicia CreditAttribution: MariaAlicia commented
Assigned: MariaAlicia » Unassigned
Issue summary: View changes
FileSize
removed_unnecessary_programming_from_previous_patch-1574512-18.patch776 bytes

Hi,
This is my first time working with Drupal as I am still in high school and have only recently learnt about working in open source. I have never coded anything using JavaScript until two days ago so my level of understanding is quite basic.

Attached is a patch that addresses the concerns in #16

nod_’s picture

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

Hey congrats! and welcome!

So there is one problem with the patch. Thing is that javascript doesn't like strings going over multiple lines.

If you visit the this page: /admin/structure/block you should have a javascript error in the console that says something like SyntaxError: unterminated string literal.

That's because you have new lines within your string. if you can put all the Ids on the same line, we're good to go :)

The last submitted patch, 18: removed_unnecessary_programming_from_previous_patch-1574512-18.patch, failed testing.

The last submitted patch, 18: removed_unnecessary_programming_from_previous_patch-1574512-18.patch, failed testing.

MariaAlicia’s picture

MariaAlicia CreditAttribution: MariaAlicia commented
Status: Needs work » Needs review
FileSize
removed_unnecessary_programming_from_previous_patch-1574512-22.patch762 bytes

Re-rolled with above suggestions.

wiifm’s picture

wiifm
Primary language English
Location Wellington, NZ
CreditAttribution: wiifm commented
Status: Needs review » Reviewed & tested by the community

Have applied the patch to 8.x and can confirm there are no errors on the block layout screens coming from block.js (there is another unrelated error coming from escapeAdmin.js:

Uncaught TypeError: Cannot read property 'currentPathIsAdmin' of undefined escapeAdmin.js?v=8.0-dev:17
(anonymous function) escapeAdmin.js?v=8.0-dev:17
(anonymous function) escapeAdmin.js?v=8.0-dev:39

There appears to be an issue already open at #2169299: Fix "Cannot read property 'currentPathIsAdmin' of undefined" error in escapeAdmin.js for this.

This addresses @nod_'s concerns at #19, and I think makes an improvement to the readability of the code.

Happy to RTBC this patch.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Rtbc+1 thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia commented
Parent issue: » #1574470: Selectors clean-up