Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#22 | removed_unnecessary_programming_from_previous_patch-1574512-22.patch | 762 bytes | MariaAlicia |
Comments
Comment #1
nod_Use a CSS selector instead of traversing to get the label text
Comment #2
pounardIs that normal you loose the context parameter of the attach() function?
Comment #3
nod_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
Comment #4
droplet CreditAttribution: droplet commentedwhy 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.
Comment #5
pounardcontext 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.
Comment #6
nod_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? :)
Comment #7
pounardI didn't tested myself on my browser, but I dare to trust you about this one, the patch seems straightfoward enough :)
Comment #8
nod_Well for this one it's fine but beware of JS patches, they can bite :)
Comment #9
pounardOf course, if there is someone else to test with IE which I can't do, working fine with FF 11 and Chromium 18
Comment #10
pounard@nod_ Yes I ended up testing after 2 seconds I clicked the submit button.
Comment #11
pounardAlso 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.
Comment #12
catchLooks good to me, committed/pushed to 8.x.
Comment #14
Kiphaas7 CreditAttribution: Kiphaas7 commentedInvestigate 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?
Comment #15
oresh CreditAttribution: oresh commentedSmall patch for lighter js and some performance clean up (remove var duplication and other).
Comment #16
droplet CreditAttribution: droplet commentedTo me it seems only these 3 lines is needed.
Comment #17
MariaAlicia CreditAttribution: MariaAlicia commentedAssigned to myself so that I can work on it.
Comment #18
MariaAlicia CreditAttribution: MariaAlicia commentedHi,
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
Comment #19
nod_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 likeSyntaxError: 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 :)
Comment #22
MariaAlicia CreditAttribution: MariaAlicia commentedRe-rolled with above suggestions.
Comment #23
wiifmHave 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 fromescapeAdmin.js
: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.
Comment #24
nod_Rtbc+1 thanks!
Comment #25
webchickCommitted and pushed to 8.x. Thanks!
Comment #27
Manuel Garcia CreditAttribution: Manuel Garcia commented