This is a follow-up to #3023256: Radio style updates (this one holds the actual patch) and #3023293: Checkboxes style update (this one contains the task).

Problem/Motivation

Remove overrides of tableselect.js and media_library.view.js files when core issues are resolved:

Drupal core issue #3024975: Add Drupal JavaScript theme function for checkbox (ore something similar) gets in.

Proposed resolution

  1. Remove js/claro.tableselect.[es6.?].js
  2. Remove claro/drupal.tableselect library from claro.libraries.yml and from claro.info.yml's libraries extends
  3. Remove js/claro.media_library.view.[es6.?].js
  4. Remove claro/media_library.view library from claro.libraries.yml and from claro.info.yml's libraries extends
  5. Remove claro/drupal-theme dependency from claro/global-styling and use the library as the extension of the core/drupal (or an equivalent) library if it's feasible
CommentFileSizeAuthor
#10 Screen Shot 2019-10-08 at 15.00.51.png32.66 KBlauriii
#10 Screen Shot 2019-10-08 at 15.00.45.png12.87 KBlauriii
#10 Screen Shot 2019-10-08 at 15.20.43.png30.23 KBlauriii
#10 Screen Shot 2019-10-08 at 15.20.32.png14.16 KBlauriii
#9 claro-clean-up-3024996-9.patch11.47 KBlauriii
#8 3085249-2.patch11.75 KBlauriii
#5 diff-media_library_view.txt1.44 KBeffulgentsia

Comments

huzooka created an issue. See original summary.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Issue summary: View changes
Parent issue: #3024975: Add Drupal JavaScript theme function for checkbox »
Related issues: +#3024975: Add Drupal JavaScript theme function for checkbox, +#3082598: Add theme function for form labels in JavaScript

#3024975: Add Drupal JavaScript theme function for checkbox is already fixed, but label/description and the form-item wrapper will be added in a separate issue.
Added #3082598: Add theme function for form labels in JavaScript.

Note:#3024975: Add Drupal JavaScript theme function for checkbox breaks our existing workaround (with Drupal core 8.8.x).

effulgentsia’s picture

effulgentsia commented

Why does this need to be postponed on #3082598: Add theme function for form labels in JavaScript? At least on the 8.x-2.x branch, I don't see anything in Claro's override of tableselect.js or media_library.view.js that depends on that issue. What am I missing?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

@effulgentsia The media_library.view.js renders a label. We override the markup to match with Claro.

effulgentsia’s picture

effulgentsia commented
StatusFileSize
new diff-media_library_view.txt1.44 KB

Is #4 true for the 2.x branch?

Here's the output I get from diff core/modules/media_library/js/media_library.view.es6.js themes/claro/js/claro.media_library.view.es6.js.

The only change I see is the removal of the js- prefix from selectors, but that looks like a regression to me (per #3079738-33: Add Claro administration theme to core) rather than something Claro intends.

effulgentsia’s picture

effulgentsia commented 
<         const $label = $(
<           '<label class="media-library-select-all"></label>',
<         ).text(Drupal.t('Select all media'));
---
>         const $label = $('<label class="media-library-select-all"></label>').text(Drupal.t('Select all media'));

In particular, that looks like no change at all to me.

lauriii credited Wim Leers.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Postponed » Needs review
StatusFileSize
new 3085249-2.patch11.75 KB

Hmm, I think @effulgentsia is right. It doesn't seem like we actually change the label markup.

I closed #3085249: [PP-1] Remove work-arounds that were blocked on #3024975 and #3082598 as a duplicate to this. Wim posted a patch on #3085249-2: [PP-1] Remove work-arounds that were blocked on #3024975 and #3082598 and I'm just uploading it here.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
StatusFileSize
new claro-clean-up-3024996-9.patch11.47 KB

This addresses also #3084724: Regression on table select checkbox.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue summary: View changes
StatusFileSize
new Screen Shot 2019-10-08 at 15.20.32.png14.16 KB
new Screen Shot 2019-10-08 at 15.20.43.png30.23 KB
new Screen Shot 2019-10-08 at 15.00.45.png12.87 KB
new Screen Shot 2019-10-08 at 15.00.51.png32.66 KB

It seems these are both broken before applying the patch:

And after applying to patch the result is as expected:

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Wow, so not only less JS in Claro, so less code to maintain and a simpler core patch in #3079738: Add Claro administration theme to core, but also bugs fixed!? Yes, please! 🙏🥳🚢

  • lauriii committed d88258b on 8.x-2.x 
    Issue #3024996 by lauriii, effulgentsia, Wim Leers: Remove tableselect....
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 8.x-1.x-dev » 8.x-2.x-dev
Status: Reviewed & tested by the community » Fixed 
 delete mode 100644 js/claro.media_library.view.es6.js
 delete mode 100644 js/claro.media_library.view.js
 delete mode 100644 js/claro.tableselect.es6.js
 delete mode 100644 js/claro.tableselect.js

🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥

Committed and pushed to 8.x-2.x.

Status: Fixed » Closed (fixed)

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