drupal_add_tabledrag() does some funky stuff there, and even has a dependency on the jquery.cookie library.

Tabledrag itself should be registered and used as a library; drupal.tabledrag.

CommentFileSizeAuthor
#12 system-tabledrag-to-library-1415828-12.patch1.33 KBdamiankloip
#6 core-js-tabledrag-lib-1415828-6.patch4.35 KBnod_
#3 system-tabledrag-to-library-1415828-3.patch1.18 KBpatrickd
#1 system-tabledrag-to-library-1415828-1.patch1.44 KBBJ___

Comments

BJ___’s picture

BJ___ commented
StatusFileSize
new system-tabledrag-to-library-1415828-1.patch1.44 KB

Here is a patch :)
Could you elaborate a little bit on "Funky stuff" ? Funky like John Coltrane ?
Is there something more to be done here ?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Needs review
patrickd’s picture

patrickd commented
StatusFileSize
new system-tabledrag-to-library-1415828-3.patch1.18 KB 
--- a/core/modules/system/system.module
+++ b/core/modules/system/system.module
@@ -1148,7 +1148,7 @@ function system_library_info() {
       'core/misc/states.js' => array('group' => JS_LIBRARY, 'weight' => 1),
     ),
   );
-
+__

looks good, but you added some whitespaces at system.module, make sure to remove all auto-indents your editor creates

BJ___’s picture

BJ___ commented

I don't think any whitespace was added. I think it's just that there was one space removed, and then one space added. Thus - then + .
But it could be misread and shouldn't be there anyway. The patch passed simple test.
Brings up an interesting point though. The reason that it was there was because I wasn't quite sure where to place it.
The alphabetical order is a bit wonky in the system module. I'm not sure if that is intended or needs to be cleaned up. Maybe worth a look.

tecnomarcos’s picture

tecnomarcos commented
nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-js-tabledrag-lib-1415828-6.patch4.35 KB

updated and removed the dependency on jquery.cookie by using localStorage.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Closed (duplicate)

Moving over to #1524414: Rewrite tabledrag.js to use jQuery UI

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Closed (duplicate) » Needs review

Hrm. That issue looks like a major effort, which might even take months to complete, given how much research and also testing will have to be done. Contrary to that, #1415828: Tabledrag should be a library is a very trivial refactoring (and the replacement of jquery cookie was never within the scope of that issue).

I'd really prefer to be on the safe side for D8 and do the trivial refactoring first, which can land at any time and does not require manual testing. Therefore, I'm going to re-open the issue.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Patch in #3 looks fine - we're just missing the dependency on jquery.cookie in the library definition, which then allows us to remove all the manual additions of the jquery.cookie library wherever tabledrag is added.

nod_’s picture

nod_
Primary language French
Location Lille
commented

Yeah remember now, the localstorage thing was remove the need for adding jquery.cookie to the party since it's not needed for D8.

can reroll without it if you still want to go with cookies.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

yeah, let's just do the change in PHP here. And since that only touches PHP code, that makes the essential difference to #1415828: Tabledrag should be a library ;)

damiankloip’s picture

damiankloip commented
StatusFileSize
new system-tabledrag-to-library-1415828-12.patch1.33 KB

This should do it then?

nod_’s picture

nod_
Primary language French
Location Lille
commented

looks good to me.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community
sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I double-checked that this is the only invocation. Good to go.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

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