Issue #2473943 by mortendk, rteijeiro, mu5a5hi, Manjit.Singh, rachel_norfolk, Cottser, LewisNyman: Prefix form-file and form-managed-file classes with js-

Task

Prefix form-file and form-managed-file classes with js- to separate classes needed for JavaScript functionality from those used for styling and make it clear which classes can safely be removed without breaking JavaScript functionality.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing

Steps to test

  1. Navigate to /node/add/article
  2. Upload an image
  3. Remove the image

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Issue priority Normal
Unfrozen changes Unfrozen because it mostly just affects templates and JS which are not frozen.
Prioritized changes The main goal of this issue is to improve themer experience and arguably it reduces fragility where JavaScript and markup intersect.
Disruption Shouldn't be too disruptive as it is mostly affecting CSS classes that are added to markup. Themes extending Classy will only have classes added. Themes not extending Classy will have classes removed but they can be added back via template overrides.

User interface changes

None for themes extending Classy. Possible visual changes for other themes.

API changes

n/a

CommentFileSizeAuthor
#47 prefix_form_file_and-2473943-47.patch8.79 KBManjit.Singh
#43 interdiff-2473943-24-41.txt612 bytesManjit.Singh
#41 prefix_form_file_and-2473943-41.patch8.96 KBManjit.Singh
#25 interdiff-2473943-21-24.txt427 bytesManjit.Singh
#25 interdiff-2473943-13-21.txt4.51 KBManjit.Singh
#24 prefix_form_file_and-2473943-24.patch8.95 KBManjit.Singh
#21 prefix_form_file_and-2473943-21.patch8.93 KBManjit.Singh
#19 reroll-patch.png65.91 KBManjit.Singh
#13 prefix_form_file_and-2473943-13.patch8.91 KBmu5a5hi
#3 interdiff.txt1.25 KBstar-szr
#3 2473943-3.patch8.91 KBstar-szr
#1 interdiff-2473943-form-file.txt6.45 KBstar-szr
#1 2473943-form-file-additional.patch8.32 KBstar-szr
#1 2473943-form-file-split.patch2.49 KBstar-szr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Active » Needs review
Issue tags: +banana
FileSize
2473943-form-file-split.patch2.49 KB
2473943-form-file-additional.patch8.32 KB
interdiff-2473943-form-file.txt6.45 KB

Initial patch split from the parent, some additional changes from grepping, and interdiff between the two.

Status: Needs review » Needs work

The last submitted patch, 1: 2473943-form-file-additional.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs work » Needs review
FileSize
2473943-3.patch8.91 KB
interdiff.txt1.25 KB

Rerolled for #2407565: Consensus Banana Phase 1, cleanup, interdiff shows the template changes that have been moved from preprocess.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs manual testing

I manually tested the patch and it works. I also grepped for more changes and couldn't find any.

+++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
@@ -317,8 +317,8 @@ function testImageFieldSettings() {
+    $this->assertNoRaw('<input multiple type="file" id="edit-' . strtr($field_name, '_', '-') . '-2-upload" name="files[' . $field_name . '_2][]" size="22" class="js-form-file form-file">');
+    $this->assertRaw('<input multiple type="file" id="edit-' . strtr($field_name, '_', '-') . '-3-upload" name="files[' . $field_name . '_3][]" size="22" class="js-form-file form-file">');

Should we remove the non-functional class from the test?

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs work » Needs review

All tests are currently using Classy so it makes sense (and is necessary) to keep both classes in the tests at least until #2352949: Deprecate using Classy as the default theme for the 'testing' profile.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Reviewed & tested by the community

Ok sounds good to me, just checking

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs review 
  1. +++ b/core/themes/classy/templates/content-edit/file-managed-file.html.twig
@@ -10,6 +10,12 @@
+    'form-managed-file',

    If classy sets the form-managed-file class now.

  2. +++ b/core/modules/file/file.field.inc
@@ -27,8 +27,8 @@ function template_preprocess_file_widget(&$variables) {
+  $variables['attributes'] = array('class' => array('file-widget', 'js-form-managed-file', 'form-managed-file', 'clearfix'));

    Then why also set it here? 

  3. +++ b/core/modules/image/image.field.inc
@@ -21,7 +21,7 @@
+  $variables['attributes'] = array('class' => array('image-widget', 'js-form-managed-file', 'form-managed-file', 'clearfix'));

    And here?

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

@catch because those are different bits of markup coming from three different theme hooks :/

  • file_managed_file
  • file_widget
  • image_widget
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented
Issue summary: View changes

Adding suggested commit message.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Reviewed & tested by the community

Nice

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: 2473943-3.patch, failed testing.

mu5a5hi’s picture

mu5a5hi CreditAttribution: mu5a5hi as a volunteer commented

At Drupalcon LA. Going to try to re-roll the patch.

mu5a5hi’s picture

mu5a5hi CreditAttribution: mu5a5hi as a volunteer commented
FileSize
prefix_form_file_and-2473943-13.patch8.91 KB

Automerged, Worked with no conflicts.

mu5a5hi’s picture

mu5a5hi CreditAttribution: mu5a5hi as a volunteer commented
Status: Needs work » Needs review
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Reviewed & tested by the community

Thanks, the two patches are identical.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented
Issue summary: View changes

Adding @mu5a5hi to the suggested commit message, thanks for sprinting with us at DCLA!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

This needs a reroll...

git ac https://www.drupal.org/files/issues/prefix_form_file_and-2473943-13.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  9120  100  9120    0     0  21580      0 --:--:-- --:--:-- --:--:-- 31888
error: patch failed: core/modules/file/file.js:68
error: core/modules/file/file.js: patch does not apply
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented
Issue tags: +Novice

Tagging as novice for the reroll.

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
FileSize
reroll-patch.png65.91 KB

@alexpott @cottser I am applying #13 patch locally and it is working fine ;)

reroll-patch.png

Getting No issues while apply !! Please correct if i am wrong ?

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented

@Manjit.Singh make sure to pull, the conflict here is related to another patch that was committed a few hours ago or so.

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
Status: Needs work » Needs review
FileSize
prefix_form_file_and-2473943-21.patch8.93 KB

@cottser yup forget to pull , Here is a rerolled patch :)

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented
Status: Needs review » Needs work
Issue tags: -Needs reroll

@Manjit.Singh thanks! I think we should add the form-managed-file back to core's file-managed-file.html.twig because core/modules/file/css/file.admin.css has several usages.

So another novice task would be to create a new patch and interdiff with that change!

See #2473955-9: Prefix form-wrapper classes with js- for the relevant discussion.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented
Issue summary: View changes

Adding @Manjit.Singh to the suggested commit message.

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
FileSize
prefix_form_file_and-2473943-24.patch8.95 KB

adding class as per suggestion in #22

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
FileSize
interdiff-2473943-13-21.txt4.51 KB
interdiff-2473943-21-24.txt427 bytes

adding a interdiff for #21 and #24

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
Status: Needs work » Needs review
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented

@Manjit.Singh to answer your question in IRC, rerolls don't need interdiffs :)

Edit: and thanks! I don't think I should re-RTBC this since I worked on it but it looks good to me.

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
Status: Needs review » Reviewed & tested by the community
cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Reviewed & tested by the community » Needs review

@Manjit.Singh For the same reasons as Cottser, perhaps someone else should peer review this.

Also, I hate to be a nudge but if you mark an issue RTBC it will help the community and the committers if you explain how you came to that conclusion.

mortendk’s picture

mortendk CreditAttribution: mortendk as a volunteer commented
Status: Needs review » Reviewed & tested by the community

tested both in seven & stark it works as it should :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: prefix_form_file_and-2473943-24.patch, failed testing.

mortendk’s picture

mortendk CreditAttribution: mortendk as a volunteer commented

huh ??

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented
Status: Needs work » Needs review

Seems flukey, one fail in Drupal\Tests\simpletest\Functional\BrowserTestBaseTest. I'll hit re-test.

Cottser queued 24: prefix_form_file_and-2473943-24.patch for re-testing.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Reviewed & tested by the community

Seems legit

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: prefix_form_file_and-2473943-24.patch, failed testing.

davidhernandez queued 24: prefix_form_file_and-2473943-24.patch for re-testing.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented
Status: Needs work » Reviewed & tested by the community

Was RTBC before. Not sure if we are getting random testbot fails here.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented

For the record the last one was a failed to do run-tests.sh type error. Didn't see the detailed results.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work

Looks like a reroll has gone awry... 

+++ b/core/modules/file/file.js
@@ -68,12 +68,12 @@
+      $context.find('.js-form-managed-file .form-submit').on('mousedown', Drupal.file.progressBar);

Should be .js-form-submit

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
FileSize
prefix_form_file_and-2473943-41.patch8.96 KB

replacing .form-submit with .js-form-submit

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
Status: Needs work » Needs review
Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
FileSize
interdiff-2473943-24-41.txt612 bytes

uploading interdiff

yogen.prasad’s picture

yogen.prasad CreditAttribution: yogen.prasad commented
Status: Needs review » Reviewed & tested by the community

Working Fine

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 41: prefix_form_file_and-2473943-41.patch, failed testing.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer commented
Issue tags: +Needs reroll
Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
FileSize
prefix_form_file_and-2473943-47.patch8.79 KB

rerolling a patch

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
Status: Needs work » Needs review
Issue tags: -Novice, -Needs reroll
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Reviewed & tested by the community

Great, the patch looks correct.

alexpott’s picture

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

Thanks adding the beta evaluation to the issue summary. Committed 96aaa70 and pushed to 8.0.x. Thanks!

  • alexpott committed 96aaa70 on 8.0.x 
    Issue #2473943 by Manjit.Singh, Cottser, mu5a5hi, LewisNyman, mortendk,...

Status: Fixed » Closed (fixed)

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