Adding this as a follow up. I just wanted a place to put preprocess functions we might have missed in the other issues. They can all be done in one patch, if at all, since there don't seem to be many. This will, hopefully, be the last issue fixed, which would mean phase 1 is complete.

This is what I see so far from searching:

core/includes/menu.inc
template_preprocess_menu_local_task
template_preprocess_menu_local_action (the attributes go into a #type link render array)

core/includes/theme.inc
template_preprocess_datetime_form
template_preprocess_html (db_is_active is available at the template so we might be able to move that db-offline class.)
template_preprocess_field_multiple_value_form (all classes are used to build a #type table render array)

core/modules/file/file.field.inc
template_preprocess_file_widget_multiple (all classes are used to build a #type table render array)

core/modules/file/file.module
template_preprocess_file_managed_file (form-managed-file is required for JS)

These may end up not needing a fix. Discuss/analyze and strike it out if it doesn't need a fix. If a new one is found, add it to the summary.

Suggested commit message:
Issue #2407565 by epari.siva, davidhernandez, akalata, pakmanlh, lauriii, vedpareek, brianperry: Consensus Banana Phase 1, cleanup

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Issue priority Normal because nothing is broken
Unfrozen changes Unfrozen because it only changes theme functions and templates, which are not frozen.
Prioritized changes The main goal of this issue is to improve themer experience, and is the last component of a META.
Disruption Minor disruption to template files that are changed inthis issue, but easily corrected.
Files: 
CommentFileSizeAuthor
#45 interdiff-240756-44to45.txt532 bytesdavidhernandez
#45 consensus_banana_phase-2407565-45.patch5.37 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,577 pass(es). View
#44 interdiff-2407565-41-44.txt750 bytesakalata
#44 consensus_banana_cleanup-2407565-44.patch5.37 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,389 pass(es). View
#41 interdiff-2407565-37-41.txt2.87 KBakalata
#41 consensus_banana_cleanup-2407565-41.patch4.57 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,427 pass(es). View
#37 consensus_banana_phase-2407565-37.patch3.25 KBepari.siva
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,474 pass(es), 11 fail(s), and 10 exception(s). View
#37 interdiff-34-37.txt856 bytesepari.siva
#34 consensus_banana_phase-2407565-33.patch3.25 KBepari.siva
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,468 pass(es), 11 fail(s), and 10 exception(s). View
#34 interdiff-31-33.txt528 bytesepari.siva
#31 consensus_banana_phase-2407565-31.patch3.24 KBepari.siva
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,477 pass(es), 11 fail(s), and 10 exception(s). View
#22 interdiff-20-22.txt1.92 KBpakmanlh
#22 consensus_banana_phase-2407565-22.patch3.24 KBpakmanlh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,419 pass(es). View
#20 interdiff-18-20.txt986 bytespakmanlh
#20 consensus_banana_phase-2407565-20.patch1.32 KBpakmanlh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,591 pass(es). View
#18 consensus_banana_phase-2407565-18.patch2.28 KBvedpareek
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,695 pass(es). View
#11 consensus_banana_phase-2407565-11.patch2.28 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,775 pass(es). View
#9 consensus_banana_phase-2407565-9.patch2.35 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,753 pass(es), 11 fail(s), and 10 exception(s). View
#7 phase1cleanup-2.patch2.26 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,583 pass(es). View
#5 phase1cleanup.patch2.21 KBdavidhernandez
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,272 pass(es), 16 fail(s), and 29,000 exception(s). View

Comments

mgifford’s picture

What is this postponed on?

davidhernandez’s picture

Just waiting until the all the phase 1 (#2322163: [meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates.) child issues are done. Right now, we're really just waiting to get the Views issue done. CKEditor, if we're lucky, but that one might be permanently postponed.

I was using this for cleanup, so we could fix remaining problems in one patch instead of chasing them down individually.

DickJohnson’s picture

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'),
    );

DOM
search

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.

davidhernandez’s picture

Status: Postponed » Active

I'm unpostponing this now that the Views issue is done. The only outstanding issue is the one for Ckeditor, but we may not get to it as it is still postponed on something else. We can work on this one now, which is just clean up of anything we missed.

davidhernandez’s picture

Status: Active » Needs review
FileSize
2.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,272 pass(es), 16 fail(s), and 29,000 exception(s). View

Gave this a start.

I did not make changes to template_preprocess_menu_local_action, template_preprocess_field_multiple_value_form, and template_preprocess_file_widget_multiple. They all seem like they might require a little re-arranging, especially the ones that involve links. It may not be worth the effort. The UpdateManagerUpdate stuff may not be something we can change either. It looks to build out a table and use the standard table template, not its own.

The functions I didn't touch were ones we've chosen not to modify or fall in the same category of functions building out tables or something else complicated.

Status: Needs review » Needs work

The last submitted patch, 5: phase1cleanup.patch, failed testing.

davidhernandez’s picture

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

Just moved the active variable and added a comment in the template.

lauriii queued 7: phase1cleanup-2.patch for re-testing.

lauriii’s picture

FileSize
2.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,753 pass(es), 11 fail(s), and 10 exception(s). View

Rerolled the patch

Status: Needs review » Needs work

The last submitted patch, 9: consensus_banana_phase-2407565-9.patch, failed testing.

lauriii’s picture

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

Currently taking a shot at reviewing the most recent patch.

brianperry’s picture

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.

lauriii’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes

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.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Novice
Cottser’s picture

Issue tags: +Needs reroll

This also needs a reroll now.

vedpareek’s picture

Status: Needs work » Needs review
FileSize
2.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,695 pass(es). View

Rerolled.

epari.siva’s picture

Issue tags: -Needs reroll
pakmanlh’s picture

FileSize
1.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,591 pass(es). View
986 bytes

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

davidhernandez’s picture

Status: Needs review » Needs work

Regarding form-managed-file, we still want to remove that from preprocess. We just need to add to the core template as well as the Classy template, since it is functional. Also, the comment added to menu-local-task should also be added in the core template since that variable will be available to both.

pakmanlh’s picture

Status: Needs work » Needs review
FileSize
3.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,419 pass(es). View
1.92 KB

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

Status: Needs review » Needs work

The last submitted patch, 22: consensus_banana_phase-2407565-22.patch, failed testing.

pakmanlh’s picture

Status: Needs work » Needs review

I passed the test which failed in local.

akalata’s picture

Status: Needs review » Needs work

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

@akalata, you would need to determine what condition makes the respective CSS class appear in the page markup and then make sure it is working the same after the patch. Menu local task, for example, is adding the 'active' class for an active menu item. It will be on the <li>.

Yes, this is still missing the change to template_preprocess_html. I believe the only class to move is 'db-offline'. To test that you need to make the db connection fail (,put a bad password in your settings file maybe) but still get a page to load. You might have to change your PHP settings to get it to continue on error, so you don't just get a white screen.

akalata’s picture

@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.

davidhernandez’s picture

Tabs should work, on the user page or a node page. View, Edit, etc.

davidhernandez’s picture

Issue tags: +Needs reroll

This will at least need a reroll. The 'active' class name has been changed to 'iis-active'. See #2031641: Change active class to is-active

epari.siva’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,477 pass(es), 11 fail(s), and 10 exception(s). View

Patch rerolled.

davidhernandez’s picture

Status: Needs review » Needs work
+++ b/core/themes/classy/templates/navigation/menu-local-task.html.twig
@@ -13,4 +14,4 @@
+<li{{ attributes.addClass(active ? 'active') }}>{{ link }}</li>

The class needs changing here, as well.

The last submitted patch, 31: consensus_banana_phase-2407565-31.patch, failed testing.

epari.siva’s picture

Status: Needs work » Needs review
FileSize
528 bytes
3.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,468 pass(es), 11 fail(s), and 10 exception(s). View

Class changed.

Status: Needs review » Needs work

The last submitted patch, 34: consensus_banana_phase-2407565-33.patch, failed testing.

davidhernandez’s picture

+++ b/core/themes/classy/templates/navigation/menu-local-task.html.twig
@@ -13,4 +14,4 @@
+<li{{ attributes.addClass(active ? 'is-active') }}>{{ link }}</li>

In the preprocess function, the variable name was changed to is-active, It has to be the same here.

epari.siva’s picture

Status: Needs work » Needs review
FileSize
856 bytes
3.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,474 pass(es), 11 fail(s), and 10 exception(s). View

Fixed.

Status: Needs review » Needs work

The last submitted patch, 37: consensus_banana_phase-2407565-37.patch, failed testing.

akalata’s picture

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

@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?

akalata’s picture

Status: Needs work » Needs review
FileSize
4.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,427 pass(es). View
2.87 KB
davidhernandez’s picture

...is it okay to just override the value before the check

Normally, I would say it is better to find a real way to simulate the result, but in this case I cannot find a way to reliably test this. So forcing the variable should be ok.

davidhernandez’s picture

Status: Needs review » Needs work

menu_local_task checks out. The db-offline class was moved to the template, but was not removed from template_preprocess_html.

akalata’s picture

Status: Needs work » Needs review
FileSize
5.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,389 pass(es). View
750 bytes

Not sure what happened there.

davidhernandez’s picture

FileSize
5.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,577 pass(es). View
532 bytes

menu_local_task checks out. html checks out. file managed file checks out.

I'm uploading a patch just to add a comma to the end of the classes array. Not a big deal.

davidhernandez’s picture

Issue summary: View changes
davidhernandez’s picture

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

RTBC+1

  • webchick committed 6db8cde on 8.0.x
    Issue #2407565 by epari.siva, davidhernandez, akalata, pakmanlh, lauriii...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome work, everyone!

Committed and pushed to 8.0.x. YEAH!

Status: Fixed » Closed (fixed)

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