Problem/Motivation

Drupal core's tabledrag.js uses a hard-coded markup for the representation of tabledrag-hande. Because of this, the markup of that handle is not overridable by themes.

Proposed resolution

Introduce new Drupal.theme callback for the handle.

Remaining tasks

There are other hard-coded markups in the JavaScript file.
We should consider repairing them as well, maybe in (a) separated issue(s).

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#21 3077938-21.patch7.04 KBkomalk
#19 3077938-19_REROLL.patch7.2 KBbnjmnm
#15 interdiff_8-15.txt1.9 KBbnjmnm
#15 3077938-15.patch7.15 KBbnjmnm
#12 interdiff_8-12.txt820 bytesravi.shankar
#12 3077938-12.patch7.09 KBravi.shankar
#8 3077938-8.patch7.03 KBbnjmnm
#8 interdiff_6-8.txt7.8 KBbnjmnm
#6 interdiff_4-6.txt1.73 KBkomalk
#6 3077938-6.patch2.74 KBkomalk
#4 3077938-4-javaScript-function-for-tabledrag.patch1.22 KBsibustephen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

huzooka created an issue. See original summary.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

sibustephen’s picture

sibustephen CreditAttribution: sibustephen as a volunteer and at Acquia commented
Assigned: Unassigned » sibustephen
sibustephen’s picture

sibustephen CreditAttribution: sibustephen as a volunteer and at Acquia commented
FileSize
3077938-4-javaScript-function-for-tabledrag.patch1.22 KB

Attaching patch for above issue.
Added a `tabledrag` function, which can be reused in generic.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

komalk’s picture

komalk CreditAttribution: komalk at Material for Drupal India Association commented
Status: Active » Needs review
FileSize
3077938-6.patch2.74 KB
interdiff_4-6.txt1.73 KB

Status: Needs review » Needs work

The last submitted patch, 6: 3077938-6.patch, failed testing. View results

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
FileSize
interdiff_6-8.txt7.8 KB
3077938-8.patch7.03 KB

Addressed multiple issues with the patch.
= The functions were not theme functions,
- The header variable was a string when the code below it was expecting a jQuery object,
- Did not pass JS code standards

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented

Was wrong about "The functions were not theme functions" in #8, they were extending Drupal.theme. Going to see if it's preferable to continue that pattern or if it's better for new theme functions to not use jQuery when possible.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Issue tags: +JavaScript
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer and at Smile commented
Status: Needs review » Needs work 
+++ b/core/misc/tabledrag.js
@@ -994,4 +994,8 @@ function _typeof(obj) { "@babel/helpers - typeof"; if (typeof Symbol === "functi
+  Drupal.theme.tableDragHandle = function () {
+    return "<a href=\"#\" title=\"".concat(Drupal.t('Drag to re-order'), "\"\n        class=\"tabledrag-handle\"><div class=\"handle\">&nbsp;</div></a>");
+  };

That should be in the $.extends above like the other theme functions.

ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented
Status: Needs work » Needs review
FileSize
3077938-12.patch7.09 KB
interdiff_8-12.txt820 bytes

Here I have tried to address comment #11, please review.

ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented
Assigned: sibustephen » Unassigned

Looks like @sibustephen is not working anymore on this issue.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer and at Smile commented
Status: Needs review » Needs work

The file that needs to be changed is the .es6.js file, see: https://www.drupal.org/docs/frontend-developer-tools-for-drupal-core#s-j...

No need to add a new, $.extends, you can use the one that's already there a few lines above.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
FileSize
3077938-15.patch7.15 KB
interdiff_8-15.txt1.9 KB

Addresses #11. Interdiffs from #8 as the change is more apparent than were it diffed from #12

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Issue tags: +Needs change record
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Issue tags: -Needs change record

Added CR https://www.drupal.org/node/3161073

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Needs work
Issue tags: +Needs reroll
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3077938-19_REROLL.patch7.2 KB

Rerolled

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Needs work 
+++ b/core/misc/tabledrag.es6.js
@@ -1714,6 +1712,7 @@
+<<<<<<< ours

It seems like the reroll is not 100% correct

komalk’s picture

komalk CreditAttribution: komalk at Material for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3077938-21.patch7.04 KB

Worked on #20.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer and at Smile commented
Status: Needs review » Reviewed & tested by the community

Looks good to me, Still works.

The linebreak is inside the html tag between 2 attributes, so there is no visual change because of it.

rename of the $handle var follow conventions and is done locally so no impact on the rest of the code.

  • lauriii committed 4790a87 on 9.1.x 
    Issue #3077938 by bnjmnm, komalkolekar, ravi.shankar, sibustephen, nod_...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Fixed

Committed 4790a87 and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Issue tags: -JavaScript +JavaScript

published the change record