Consensus Banana Phase 1, cleanup

What is this postponed on?

This is copy/paste from #2329829: Move update classes from preprocess to templates based on what @davidhernandez said.

Update module

I'm not sure how much work there is

While I was working on #2408499: Rewrite update components inline with our CSS standards I noticed that we still have classes at least in core/modules/update/src/Form/UpdateManagerUpdate.php

example:

1. rows 245-252
    $headers = array(
      'title' => array(
        'data' => $this->t('Name'),
        'class' => array('update-project-name'),
      ),
      'installed_version' => $this->t('Installed version'),
      'recommended_version' => $this->t('Recommended version'),
    );

I also noticed that a lot of markup is coming from somewhere else than templates, so when we start to add new classes to update-modules table-elements I'm pretty sure we will get even more classes to files that are not templates. I haven't yet figure out how much we have these, but my guess is that quite a lot.

As I tried to point out there: https://www.drupal.org/node/2408499#comment-9591331 testing these templates, CSS and which classes are being used and where is relatively difficult, so we should double check things.

Rerolled the patch

Currently taking a shot at reviewing the most recent patch.

Wasn't able to fully confirm this.

I'm confident that for menu local tasks the active class is coming from menu-local-task.html.twig After applying the patch I still see the active class - for example on the content tab at admin/content. To confirm that this was in fact coming from the twig template I edited the template to remove the active class. After doing so, the active class no longer remains and no tab is styled as active.

On the other hand, I don't believe that the change in file-managed-file.html.twig is actually being used. After applying the patch I do still see the form-managed-file class - for example on the image file widget on the article node edit form. But if I change the class in the template, I don't see the resulting change to the class for the form widget. Best I can tell in this case the class is actually coming from template_preprocess_file_widget in file.field.inc But even just removing the class there didn't result in the class in the template being used. So I'm missing something, or possibly even trying to use the wrong approach to verify this. Either way, I'm not sure that this portion of the patch is working as intended.

I updated the issue summary to cross out ones that can't be banana-fied and explained why for each one.

form-managed-file in template_preprocess_file_managed_file() is actually required for JS, so it might make sense to move to #2431671: [meta] Add in js- prefixed classes for separation of JS & CSS functionality, although that patch is already quite large.

That would just leave the local task cleanup, and the template_preprocess_html should be done, very banana-able.

This also needs a reroll now.

Rerolled.

I attached a new simplified patch following the #15 instructions.

Ups, sorry. I attached again a new patch, hope this one will be better.

I passed the test which failed in local.

1. Reviewer question: What should I be looking at to test the menu_local_task change?
2. It doesn't look like the change needed in template_preprocess_html has been addressed yet?
3. Again, how can I test this? (Or put another way, how do you know that the changes you made had the effect you intended?)

@davidhernandez - yep, that's exactly my question. Since there aren't "menu-local-task" classes anymore, I don't know which menu is the local task menu -- primary tabs? secondary tabs? While I could dig around in the code, changing classes until I see what is being acted upon so that I know I'm in the right place, that's sort of a waste of my time when the person who wrote the patch should know that information and can share it.

Patch rerolled.

Class changed.

Fixed.

1. + $variables['is-active'] = TRUE;
   To match coding standards, this variable should use an underscore instead of a hyphen.
2. The docblocks at the top of core/modules/system/templates/menu-local-task.html.twig and core/themes/classy/templates/navigation/menu-local-task.html.twig need to be updated with the correct variable name.

@davidhernandez, for testing out the template_preprocess_html > html.html.twig change, is it okay to just override the value before the check (ie temporarily hardcoding $variables['db_is_active'] = FALSE;) to make the class show up?

It won't change the visual display of the page to what db-offline really looks like (since there won't be an error message for other content), but DOES apply the class, which is really all we're looking to test in this case?

Not sure what happened there.

RTBC+1

Awesome work, everyone!

Committed and pushed to 8.0.x. YEAH!