Problem/Motivation

Remove non essential classes from core - the classes are in classy, cores templates should be as clean as possible.
simpletest should be done with classy theme, not stark

Proposed resolution

See parent issue(s) and related issue(s) for details,

Twig Templates to modify

  • core/modules/system/templates/feed-icon.html.twig
  • core/modules/system/templates/field-multiple-value-form.html.twig
  • core/modules/system/templates/field.html.twig
  • core/modules/system/templates/fieldset.html.twig
  • core/modules/system/templates/form-element-label.html.twig
  • core/modules/system/templates/form-element.html.twig
  • core/modules/system/templates/form.html.twig

Remaining tasks

User interface changes

API changes

Files: 
CommentFileSizeAuthor
#39 2407727-39-remove-classes-f-templates.patch3.87 KBmdrummond
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,539 pass(es). View
#29 node-teaser-before.png371.62 KBrteijeiro
#29 node-teaser-after.png371.62 KBrteijeiro
#29 node-before.png773.25 KBrteijeiro
#29 node-after.png773.3 KBrteijeiro
#29 form-before.png506.62 KBrteijeiro
#29 form-after.png505.68 KBrteijeiro
#28 f-templates-6.diff7.14 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,270 pass(es). View
#23 interdiff-f.txt1.95 KBmortendk
#23 f-templates-5.diff8.4 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch f-templates-5.diff. Unable to apply patch. See the log in the details link for more information. View
#19 f-templates-4.diff8.35 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,217 pass(es). View
#19 f-interdiff-3-4.txt1.92 KBmortendk
#14 form-after.png466.98 KBrteijeiro
#14 form-before.png466.57 KBrteijeiro
#13 f-templates-3.diff10.14 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,822 pass(es). View
#10 cleanup-templates-f-2.diff7.71 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,691 pass(es), 5 fail(s), and 0 exception(s). View
#6 cleanup-templates-f.diff8.25 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch cleanup-templates-f.diff. Unable to apply patch. See the log in the details link for more information. View
#4 remove_classes_from-2407727-4.patch5.06 KBdavidhernandez
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,229 pass(es), 5 fail(s), and 0 exception(s). View
#1 issue-2407727-1.patch14 KBSivaji
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,932 pass(es). View

Comments

Sivaji’s picture

Status: Active » Needs review
FileSize
14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,932 pass(es). View
davidhernandez’s picture

Status: Needs review » Postponed

Postponing this until we decide how we are moving forward. Keep an eye on #2348543: [meta] Consensus Banana Phase 2, transition templates to the Classy theme for updates. Thanks.

davidhernandez’s picture

Title: Copy system templates f*.html.twig to Classy » Remove classes from system templates f*.html.twig
Issue summary: View changes
Status: Postponed » Needs work

Un-postponing. The templates have been copied to Classy, so all we need to do is remove classes from the original templates.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
5.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,229 pass(es), 5 fail(s), and 0 exception(s). View

I left a few classes. I'm not sure where we stand on things like form-required.

Status: Needs review » Needs work

The last submitted patch, 4: remove_classes_from-2407727-4.patch, failed testing.

mortendk’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
8.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch cleanup-templates-f.diff. Unable to apply patch. See the log in the details link for more information. View

look through the css & js found a js- prefix for .form-disabled

Status: Needs review » Needs work

The last submitted patch, 6: cleanup-templates-f.diff, failed testing.

Status: Needs work » Needs review

mortendk queued 6: cleanup-templates-f.diff for re-testing.

Status: Needs review » Needs work

The last submitted patch, 6: cleanup-templates-f.diff, failed testing.

mortendk’s picture

Status: Needs work » Needs review
FileSize
7.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,691 pass(es), 5 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 10: cleanup-templates-f-2.diff, failed testing.

mortendk’s picture

looks like its testing on core and not classy ?

mortendk’s picture

Status: Needs work » Needs review
FileSize
10.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,822 pass(es). View

fixed test ensured they used classy to test on

rteijeiro’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
466.57 KB
466.98 KB

Good job, @mortendk! Keep rocking!! \m/

Form elements BEFORE

Form elements AFTER

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Something is feeling really wrong about prefixing everything that javascript touches with js- especially when the class is being used to give the element style.

mortendk’s picture

Issue tags: +jsbanana
mortendk’s picture

Status: Needs review » Needs work
mortendk’s picture

Something is feeling really wrong about prefixing everything that javascript touches with js- especially when the class is being used to give the element style.

We need this sepration for the css to not accedentially removing / overwriting somebody thats needed / provided from the js.

This means that we need to refactor the js stuff & also clean up the css to follow our codestandards

mortendk’s picture

Status: Needs work » Needs review
FileSize
1.92 KB
8.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,217 pass(es). View

after looking at the templates, js & forms we need to seperate that out to a single issue that cleans up the js dependencies in the forms, but we should do that in one patch, else its gonna be a logistical nightmare to make it work. Theres simply to many places that we need to clean up.

This patch removes the classes we can remove from core, without going down the rabbit hole of js dependency with forms

@todo create an issue with js prefix / data- attribute selectors for js functionality in core.
#2408473: Rewrite form.css inline with our CSS standards

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies and works. No visual regressions. Back to RTBC then...

mortendk’s picture

Issue summary: View changes
mortendk’s picture

mortendk’s picture

FileSize
8.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch f-templates-5.diff. Unable to apply patch. See the log in the details link for more information. View
1.95 KB
mortendk’s picture

Status: Reviewed & tested by the community » Needs review

2 classes got deleted from core, that well take care at over in #2431671: [meta] Add in js- prefixed classes for separation of JS & CSS functionality

mortendk’s picture

Issue tags: +dclondon

mortendk queued 23: f-templates-5.diff for re-testing.

Status: Needs review » Needs work

The last submitted patch, 23: f-templates-5.diff, failed testing.

mortendk’s picture

Status: Needs work » Needs review
FileSize
7.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,270 pass(es). View
rteijeiro’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
505.68 KB
506.62 KB
773.3 KB
773.25 KB
371.62 KB
371.62 KB

Looks good! Great job!

Node Teaser BEFORE

Node Teaser AFTER

Node BEFORE

Node AFTER

Node Edit Form BEFORE

Node Edit Form AFTER

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

We're removing lots of classes that people have become very accustomed to in this issue - all of these issues should share a single CR that list all of the classes removed and any added for js implementations.

mortendk’s picture

@alex should we combine that with the cr for the form cleanup ? [#2452043]

mortendk’s picture

@alex should we combine that with the cr for the form cleanup ? [#2452043]

mortendk’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

changerecord added [#2452297]
setting back to rtbc ?

webchick’s picture

Assigned: Unassigned » alexpott

Back to Alex for feedback. Although...

+++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceFormatterTest.php
@@ -177,6 +177,10 @@ public function testEntityFormatter() {
+    // Enable the Classy theme.
+    \Drupal::service('theme_handler')->install(['classy']);
+    $this->config('system.theme')->set('default', 'classy')->save();

This trend worries me a bit. It's adding noise to that test which has absolutely nothing to do with classy theme. And from the looks of it, this pattern is introduced in other tests as well.

What's the rationale for this? Did one of the other core committers recommend this approach?

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

How does the view-ui look in stark before and after this patch?

alexpott’s picture

Status: Needs review » Needs work

Also re #34 this means that the test has some assertions for classes removed by this patch. Which hints that these things need to be manually tested in stark to make sure they still work and are usable.

Putting back to needs work to ensure that (a) it is really necessary to have the tests depend on classy (b) that the necessary manual testing has been carried out and evidence posted to this issue.

[EDIT: to make sense]

mortendk’s picture

@webchick the rationale is when a test is not using classy but goes to stark for the test.

davidhernandez’s picture

It looks like all the js- prefix needing for this one has been done and it is free to work on.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
3.87 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,539 pass(es). View

The last patch was very, very old. So I tried to manually reapply the changes. It looked a few more classes had been made safe to remove, so I did my best guesses with some grepping. Some JS and tests might still need to be updated that have both js-form-required and form-required as part of the efforts to make the js version of the class. So there will probably be initial errors that can help find those problems.

This should be looked at carefully in case some of the classes are still needed.

gints.erglis’s picture

Patch applies and works. No visual regressions.

davidhernandez’s picture

Status: Needs review » Needs work

This is missing the removal from feed-icon.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.