Problem/Motivation

tableselect.js in Drupal core and media_library.view.js in Media Library insert a checkbox input HTML element. Currently, markup of these elements is not overridable by themes.

Proposed resolution

Introduce a new Drupal.theme callback for checkbox, and refactor the files above to use that for creating the markup of the checkbox.

Remaining tasks

  • Provide patch
  • JS maintainer sign-off
  • Create change record
CommentFileSizeAuthor
#16 core-checkbox_theme_js-8.8.x-3024975-16.patch9.66 KBquiron
#16 interdiff_3024975_9_16.txt3.5 KBquiron
#9 interdiff_8_9.txt2.49 KBquiron
#9 core-checkbox_theme_js-8.8.x-3024975-9.patch6.31 KBquiron
#8 interdiff_2_8.txt3.94 KBquiron
#8 core-checkbox_theme_js-8.8.x-3024975-8.patch5.69 KBquiron
#7 core-checkbox_theme_js-3024975-7.patch4.75 KBquiron
#2 core-checkbox_theme_js-3024975-2.patch5.58 KBhuzooka
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

huzooka created an issue. See original summary.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Status: Active » Needs review
FileSize
core-checkbox_theme_js-3024975-2.patch5.58 KB
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Title: Add Drupal JS theme function for checkbox » Add Drupal JavaScript theme function for checkbox
Issue summary: View changes
Issue tags: +Needs subsystem maintainer review

Would be great to get feedback from the subsystem maintainers on where they think this new theme function should be located.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Assigned: huzooka » Unassigned
nod_’s picture

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

Adding this theme function to all pages using core/drupal seems a bit overkill.

I would create a new library and have tableselect and media library depend on it.

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

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quiron’s picture

quiron
Primary language Catalan
Location Barcelona
CreditAttribution: quiron commented
FileSize
core-checkbox_theme_js-3024975-7.patch4.75 KB

Re-rolled to 8.8.x

quiron’s picture

quiron
Primary language Catalan
Location Barcelona
CreditAttribution: quiron commented
FileSize
core-checkbox_theme_js-8.8.x-3024975-8.patch5.69 KB
interdiff_2_8.txt3.94 KB

Sorry, the previous re-rolled patch was wrong. Adding the correct one.

quiron’s picture

quiron
Primary language Catalan
Location Barcelona
CreditAttribution: quiron commented
FileSize
core-checkbox_theme_js-8.8.x-3024975-9.patch6.31 KB
interdiff_8_9.txt2.49 KB

Moved the new JS code to an own library for the checkbox. So renamed JS files to 'checkbox' instead of the generic 'drupal-theme' and make tableselect and media_library view depending on it.

quiron’s picture

quiron
Primary language Catalan
Location Barcelona
CreditAttribution: quiron commented
Status: Needs work » Needs review
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Issue tags: -Needs subsystem maintainer review

@nod_ confirmed on Slack that we can remove the subsystem maintainer review tag based on #5.

lauriii’s picture

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

This change needs a change record. Drupal.org documentation on how to write a change record.

quiron’s picture

quiron
Primary language Catalan
Location Barcelona
CreditAttribution: quiron commented
Issue tags: -Needs change record +DevDaysCluj

Created a change record draft: https://www.drupal.org/node/3061281

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Needs work

It seems like we have a few more instances where checkbox is being rendered in JavaScript the old way that should be converted. The files I could find are user.permissions.es6.js and simpletest.js.

quiron’s picture

quiron
Primary language Catalan
Location Barcelona
CreditAttribution: quiron commented
Assigned: Unassigned » quiron

I will work on it today.

quiron’s picture

quiron
Primary language Catalan
Location Barcelona
CreditAttribution: quiron commented
Assigned: quiron » Unassigned
Status: Needs work » Needs review
FileSize
interdiff_3024975_9_16.txt3.5 KB
core-checkbox_theme_js-8.8.x-3024975-16.patch9.66 KB

Implementing @lauriii's feedback

rosinegrean’s picture

rosinegrean CreditAttribution: rosinegrean at PitechPlus commented
Issue tags: -DevDaysCluj +DevDaysTransylvania
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Issue tags: +Needs subsystem maintainer review

Could use a review from a JavaScript maintainer.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs subsystem maintainer review
  1. 👍 Well that issue summary sounds sensible!
  2. ➕Agreed with the "overkill" concern that JS maintainer @nod_ expressed in #5.
  3. @lauriii wrote in #11 that @nod_ was fine with this issue if his #5 recommendation was followed. Patch iterations since then do follow his recommendation. #16 merely applies the same pattern in more places, to ensure consistent rendering. The only "special" thing about them is that they both remove the form-checkbox class to ensure the exact same markup is still generated, which is certainly fine (even if said class does not exist in the theme's override of the markup). That's why I'm confident to remove the Needs subsystem maintainer review tag. The only addition is that it's removing
  4. 👍 Change record exists.
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue summary: View changes
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

Discussed this issue with @alexpott since I was concerned about the lack of label support. This is needed by Drupal.behaviors.MediaLibrarySelectAll.

Since this issue already helps us make meaningful progress, and label support could be added without breaking BC in a follow-up, we agreed on the following steps:

  1. Implement theme function for the checkbox input (this issue)
  2. Implement theme function for the form element which would provide support for labels and a wrapping div (follow-up).
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

Opened #3082598: Add theme function for form labels in JavaScript.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed 21e024e and pushed to 8.8.x. Thanks!

  • alexpott committed 21e024e on 8.8.x 
    Issue #3024975 by quiron, huzooka, lauriii, Wim Leers, nod_: Add Drupal...

Status: Fixed » Closed (fixed)

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