(First, verify that the preprocess changes have been made. #2322163: [meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates.)

  1. Copy the Twig templates from the core module's templates directory to Classy's templates directory. Include all templates, even ones without classes.
  2. Remove all classes from the core module's template. Remove all classes added with addClass and ones that are hard-coded in the template.
  • If there are classes that are required for basic functionality, discuss whether they should be kept.
  • If there is CSS from the module, or anywhere else, referring to the class, discuss removing it or moving it to Bartik&Seven. Do not move the CSS to Classy.

Twig Templates to Copy

core/modules/simpletest/templates/simpletest-result-summary.html.twig

Files: 
CommentFileSizeAuthor
#15 copy_simpletest-2349745-15.patch325 bytesbradwade
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,875 pass(es), 24 fail(s), and 0 exception(s). View
#13 copy_simpletest-2349745-13.patch959 bytesbradwade
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,356 pass(es). View
#12 Screenshot 2014-11-19 17.06.26.png81.03 KBbradwade
#1 copy_simpletest-2349745-1.patch837 bytesmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,627 pass(es). View

Comments

mortendk’s picture

Status: Active » Needs review
Issue tags: +cssbanana
FileSize
837 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,627 pass(es). View

moved & removed classes

mortendk’s picture

Issue tags: +drupalhagen
Xen’s picture

Assigned: Unassigned » Xen
Xen’s picture

Assigned: Xen » Unassigned
Status: Needs review » Reviewed & tested by the community

Works. The classy template file is picked up as it should.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

So the module provides css for simpletest-fail and simpletest-pass and this has now moved to the classy template. I'm not sure that moving this template makes a whole lot of sense since I really don't believe this is the type of css themers really want to remove or override.

mortendk’s picture

@alex i hear what your saying - But it would be inlogical to have templates thats not in the classy theme
A usecase for theming simpletest is when you build an admin theme, so it should be placed inside of classy

so yup themers want this :)

alexpott’s picture

I'm not sure I buy the "illogical argument" - why is that so?

alexpott’s picture

Also test running is something I do using stark all the time. And leaving the template in simpletest does not stop an admin theme from customising (but I'd bet at least 1 pence that no admin theme has ever wanted to remove the simpletest-pass and simpletest-fail classes.

mortendk’s picture

simpletest have class'es - we wanna make drupal core free off all the classes & they should live inside of classy - not in the module.
The reason that everything should be in classy is that we want the themer to get a complete overview of the whole system.

does that make sense ?

alexpott’s picture

I'm not sure I buy that for simpletest or views ui for that matter.

mortendk’s picture

well the question is why do we wanna have classes down in core - when classes should be defined in the classy theme.

but its minor elements that isnt essential for classy's epic succes ;)

bradwade’s picture

I'm too verbose below :) So here is a summary of my comment.

  1. We didn't remove all of the classes.
  2. We could remove them and move this information into data-attributes.
  3. Eh... Let's just leave the class in core AND copy the twig template to classy. (Patch coming...)

1. This template, that would be moved to Classy in this patch, only wraps the *summary* of the simpletest results with a class (i.e. simpletest-pass/-fail/-debug). The table below that summary, which lists out each test, still has the same classes on the tr. The table tr class is added by simpletest module in core/modules/simpletest/src/Form/SimpletestResultsForm.php currently around line 178.

        $class = 'simpletest-' . $assertion->status;
        if ($assertion->message_group == 'Debug') {
          $class = 'simpletest-debug';
        }
        $rows[] = array('data' => $row, 'class' => array($class));

So we aren't really gaining much by moving one instance of adding the class to the theme layer and leaving the other in the module.

2. Trying to make everybody happy... we could still strip classes from core AND continue to pass on this relevant data information by putting it into a data-attribute on the element, changing the above to:

        $data_attribute = $assertion->status;
        if ($assertion->message_group == 'Debug') {
          $data_attribute = 'debug';
        }
        $rows[] = array('data' => $row, 'data-simpletest-result' => $data_attribute);

And this in the simpletest twig file:

<div data-simpletest-result="{{ fail + exception == 0 ? 'pass' : 'fail' }}">
  {{ label }} {{ items|join(', ') }}
</div>

3. However I think the most expedient route and acceptable (to me at least) would be to simply leave the class in core (as per @alexpott's point). AND for thoroughness and consistency of the the Classy project, duplicate the template to Classy.

I'll have a patch to that effect shortly.

bradwade’s picture

Assigned: Unassigned » bradwade
FileSize
959 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,356 pass(es). View

Left class in core twig file per @alexpott AND copied twig file to Classy theme.

davidhernandez’s picture

Brad, check your git settings. See the section here on renaming, https://www.drupal.org/documentation/git/configure . If you copy the file, without changes, it should just give you a small diff without the file contents.

bradwade’s picture

FileSize
325 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,875 pass(es), 24 fail(s), and 0 exception(s). View

Thanks @davidhernandez. Trying again... leaving the class in core template and simply copying template to Classy.

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

I spoke to David on IRC. It seems like this patch does everything needed for this issue.

alexpott’s picture

I still think we should grant an exception to the no classes in core aim for simpletest. Why do we want to make running tests in stark painful?

alexpott’s picture

Lol... should have looked at the patch - we're not removing the classes anymore - so I wonder why we're putting this in classy. It does not seem relevant.

webchick’s picture

Status: Reviewed & tested by the community » Postponed
Issue tags: +SprintWeekend2015

It looks like alexpott and I are raising similar concerns in a couple different issues. Let's postpone this on an updated issue summary + buy-in from core maintainers at #2349759: Copy system templates to Classy.

mortendk’s picture

So i might could accept not moving This one template to classy, but thats it.
We cant have a template system that again is inconsistent all over

What were gonna do when all the templates are moved over, is putting them all in folders, so its easy to find, one of these folders would be "backend stuff" (or what we call em)
But i generally dissagrees everything must be moved to classy, lets not shake on the hand, just give us themers all em templates once n for all :)

mgifford’s picture

Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch, 15: copy_simpletest-2349745-15.patch, failed testing.

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.