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

Files: 
CommentFileSizeAuthor
#47 prefix_form_file_and-2473943-47.patch8.79 KBManjit.Singh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,815 pass(es). View
#43 interdiff-2473943-24-41.txt612 bytesManjit.Singh
#41 prefix_form_file_and-2473943-41.patch8.96 KBManjit.Singh
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch prefix_form_file_and-2473943-41.patch. Unable to apply patch. See the log in the details link for more information. View
#24 prefix_form_file_and-2473943-24.patch8.95 KBManjit.Singh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,562 pass(es). View
#13 prefix_form_file_and-2473943-13.patch8.91 KBmu5a5hi
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,519 pass(es). View
#1 2473943-form-file-split.patch2.49 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,621 pass(es). View

Comments

Cottser’s picture

Status: Active » Needs review
Issue tags: +banana
FileSize
2.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,621 pass(es). View
8.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2473943-form-file-additional.patch. Unable to apply patch. See the log in the details link for more information. View
6.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.

Cottser’s picture

Status: Needs work » Needs review
FileSize
8.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2473943-3.patch. Unable to apply patch. See the log in the details link for more information. View
1.25 KB

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

LewisNyman’s picture

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?

Cottser’s picture

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: [meta] Remove Classy testing profile dependency .

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

Ok sounds good to me, just checking

catch’s picture

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?

Cottser’s picture

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

  • file_managed_file
  • file_widget
  • image_widget
Cottser’s picture

Issue summary: View changes

Adding suggested commit message.

LewisNyman’s picture

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

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

mu5a5hi’s picture

FileSize
8.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,519 pass(es). View

Automerged, Worked with no conflicts.

mu5a5hi’s picture

Status: Needs work » Needs review
LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, the two patches are identical.

Cottser’s picture

Issue summary: View changes

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

alexpott’s picture

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
Cottser’s picture

Issue tags: +Novice

Tagging as novice for the reroll.

Manjit.Singh’s picture

FileSize
65.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 ?

Cottser’s picture

@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

Status: Needs work » Needs review
FileSize
8.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,592 pass(es). View

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

Cottser’s picture

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.

Cottser’s picture

Issue summary: View changes

Adding @Manjit.Singh to the suggested commit message.

Manjit.Singh’s picture

FileSize
8.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,562 pass(es). View

adding class as per suggestion in #22

Manjit.Singh’s picture

adding a interdiff for #21 and #24

Manjit.Singh’s picture

Status: Needs work » Needs review
Cottser’s picture

@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

Status: Needs review » Reviewed & tested by the community
cilefen’s picture

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

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

huh ??

Cottser’s picture

Status: Needs work » Needs review

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

LewisNyman’s picture

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’s picture

Status: Needs work » Reviewed & tested by the community

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

Cottser’s picture

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

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

FileSize
8.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch prefix_form_file_and-2473943-41.patch. Unable to apply patch. See the log in the details link for more information. View

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

Manjit.Singh’s picture

Status: Needs work » Needs review
Manjit.Singh’s picture

FileSize
612 bytes

uploading interdiff

yogen.prasad’s picture

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

Issue tags: +Needs reroll
Manjit.Singh’s picture

FileSize
8.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,815 pass(es). View

rerolling a patch

Manjit.Singh’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -Needs reroll
LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

Great, the patch looks correct.

alexpott’s picture

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.