Display same info for themes as for modules

Dupe of #2348793: I can not uninstall Classy theme

Or is this a follow-up, saying the work done in that issue was not enough?

No reason to be postponed

I'll work on this in the D8 MUC sprint.

Nice, it's great to make this code more reusable, though I think we will tackle the majority of it in #2408505: Rewrite module page CSS inline with our CSS standards.

I noticed that the details span the full width of the page because the theme-info class is not floated alongside the theme image. This is obvious when the details element has focus.

The spacing between the details summary and the theme title is very small. It looks a little weird. It seems like it's possible to fix this by removing the padding top from .system-projects td and putting it on .system-projects details summary

We also need to change some of the classes in the Seven modules-page.css especially:

.system-modules .details-wrapper {
  padding: 0 0 0.5em 0;
}

Dependent on #2543904: Make attributes optional in the details template to get rid of the errors.

Overall looks good and I'm +1 for doing this. We still need UX people to agree with the layout.

+++ b/core/modules/system/system.admin.inc
@@ -407,12 +416,40 @@ function template_preprocess_system_themes_page(&$variables) {
+      $version = isset($theme->info['version']) ? $theme->info['version'] : '';
+      // Add the description, along with any theme it requires.
+      $description = '';
+      $description .= '<div class="requirements">';
+      $description .= '<div class="admin-requirements">' . t('Machine name: !machine-name', array('!machine-name' => '<span dir="ltr" class="table-filter-text-source">' . $theme->getName() . '</span>')) . '</div>';
+      if ($version) {
+        $description .= '<div class="admin-requirements">' . t('Version: !module-version', array('!module-version' => $version)) . '</div>';
+      }
+      if (!empty($theme->requires)) {
+        $names = array();
+        foreach (array_keys($theme->requires) as $key) {
+          $names[] = isset($theme_infos[$key]) ? $theme_infos[$key] : $key;
+        }
+        $description .= '<div class="admin-requirements">' . t('Requires: !module-list', array('!module-list' => implode(', ', $names))) . '</div>';
+      }
+      if (!empty($theme->required_by)) {
+        $names = array();
+        foreach (array_keys($theme->required_by) as $key) {
+          $names[] = isset($theme_infos[$key]) ? $theme_infos[$key] : $key;
+        }
+        $description .= '<div class="admin-requirements">' . t('Required by: !module-list', array('!module-list' => implode(', ', $names))) . '</div>';
+      }
+      $description .= '</div>';

Otherwise things look good but this should be done in a template. We should try to avoid adding markup in PHP code as much as possible.

As requested, this updated patch fixes the test failures here instead of in #2543904: Make attributes optional in the details template.

In addition, it takes into account the comments from #11.

Regarding #13, I just reused some code for creating the module table that was above in the same file. I think fixing it properly (in both places) should be handlded in a separate issue.

And the interdiff

We shouldn't add code that we know needs to be replaced in future. We can create follow-up to make the module page follow the direction that has been taken here.

This is visually broken because I changed the classes used in the template. They might not be still the final ones because they are still confusing. Maybe @jcventura you want to tackle these and the CSS changes needed?

Removed .swp from patch

Updated mostly only the templates, and changed to short array syntax where lauriii hadn't corrected it before. (THANKS!)

Once this is commited, it will be trivial to use the same these same templates to the modules page.

Oops. Extremely minor fix.. I changed the classy template so much that I copied the default over it, and overrode part of the comment.

Bojan bat signal

Fixed the test. The theme version is now part of the collapsed details, not the title anymore.

Git complains about empty lines at the end of the template files (attached patch fixes that), otherwise it looks good.

PHP looks good - only one issue I can see:

+++ b/core/modules/system/system.admin.inc
@@ -451,5 +484,4 @@ function template_preprocess_system_themes_page(&$variables) {
+}
\ No newline at end of file

Whitespace issue here

This is looking pretty good from a pure code review:

1. +++ b/core/modules/system/css/system.admin.css
   @@ -76,84 +76,87 @@ small .admin-link:after {
   +  overflow: hidden; /* truncates descriptions if too long */
   

   Probably needs a capital 'Truncates' and period at the end of the comment. (event though that was what it said before the patch)

2. +++ b/core/modules/system/templates/system-project-details.html.twig
   @@ -0,0 +1,30 @@
   +    <div>{{ 'Machine name: !machine-name'|t({'!machine-name': machine_name}) }}</div>
   ...
   +    <div>{{ 'Version: !version'|t({'!version': version}) }}</div>
   

   Same here use @ instead of !

3. +++ b/core/modules/system/templates/system-themes-page.html.twig
   @@ -56,12 +56,12 @@
   -            <div class="theme-info__description">{{ theme.description }}</div>
   +            <div class="theme-info__description description">{{ theme.description }}</div>
   

   Does this really need a description class, why is the bem class not enough?

4. +++ b/core/themes/classy/templates/misc/system-project-details.html.twig
   @@ -0,0 +1,30 @@
   +    <div class="admin-requirements">{{ 'Machine name: !machine-name'|t({'!machine-name': machine_name}) }}</div>
   ...
   +    <div class="admin-requirements">{{ 'Version: !version'|t({'!version': version}) }}</div>
   

   Can we use @ placeholders here instead of !, because there shouldn't be raw variables and that doesn't do what you'd expect anymore (if it's unsafe marks the entire string as unsafe).

5. +++ b/core/modules/system/templates/system-project-details.html.twig
   @@ -0,0 +1,30 @@
   +    <div>{{ 'Requires: @extensions'|t({'@extensions': requires|join(', ')}) }}</div>
   ...
   +    <div>{{ 'Required by: @extensions'|t({'@extensions': required_by|join(', ')}) }}</div>
   

   might want |safe_join instead of |join or these will get double escaped.

6. +++ b/core/themes/classy/templates/misc/system-project-details.html.twig
   @@ -0,0 +1,30 @@
   +
   +{#
   

   Extra space at the top of the file.

As well as #28

Fixed error.

A bit of derail but I'm sure that's time to clean-up notion of module-theme in favour of Extension that is all over the core now

So probably class names will be "better" .extension__theme & .extension__module

1. +++ b/core/includes/form.inc
   @@ -243,7 +243,7 @@ function template_preprocess_fieldset(&$variables) {
   -  $variables['attributes'] = $element['#attributes'];
   +  $variables['attributes'] = isset($element['#attributes']) ? $element['#attributes'] : new Attribute();
   

   looks unrelated change

2. +++ b/core/modules/system/css/system.admin.css
   @@ -76,84 +76,87 @@ small .admin-link:after {
   -.system-modules thead > tr {
   +.system-projects thead > tr {
   

   now modules, themes and profiles are extensions. Let's keep that naming

3. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -234,6 +234,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $form['#attributes']['class'] = ['system-modules', 'system-projects'];
   
   +++ b/core/modules/system/system.admin.inc
   @@ -381,6 +381,15 @@ function template_preprocess_system_themes_page(&$variables) {
   +  $variables['attributes']['class'] = ['system-themes', 'system-projects'];
   

   both?

4. +++ b/core/modules/system/system.admin.inc
   @@ -461,5 +494,4 @@ function template_preprocess_system_themes_page(&$variables) {
      $variables['theme_groups'] = $groups;
   -}
   -
   +}
   \ No newline at end of file
   

   needs \n

5. +++ b/core/modules/system/system.module
   @@ -213,6 +213,14 @@ function system_theme() {
   +    'system_project_details' => [
   +      'variables' => [
   +        'machine_name' => [],
   +        'version' => '',
   +        'requires' => [],
   +        'required_by' => [],
   +      ],
   

   extension_details - makes more sense
   also would be great to pass "type" here

I am not convinced that this is the best way to display this. Can we explore more design directions?

I am thinking about having this in a tooltip or when you click "details"?

@Bojhan the only usage of tooltip we have in core is shortcut "star" - makes sense to re-use

Updated patch from #35
Please provide your thoughts about system-extensions modificator style

1. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -234,6 +234,10 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $form['#attributes']['class'] = [
   +      'system-extensions',
   +      'system-extensions-module',
   +    ];
   
   +++ b/core/modules/system/system.admin.inc
   @@ -381,6 +381,18 @@ function template_preprocess_system_themes_page(&$variables) {
   +  $variables['attributes']['class'] = [
   +    'system-extensions',
   +    'system-extensions-theme',
   +  ];
   

   really questionable thing, maybe better to make them shorter ['extension', 'entension--<type>'] no reason for system prefix

2. +++ b/core/modules/system/system.admin.inc
   @@ -417,12 +429,36 @@ function template_preprocess_system_themes_page(&$variables) {
   +      $description = [
   +        '#theme' => 'system_extension_details__theme',
   +        '#machine_name' => $theme->getName(),
   +        '#version' => $version,
   +        '#requires' => $requires,
   +        '#required_by' => $required_by,
   +      ];
   ...
   -      $current_theme['description'] = t($theme->info['description']);
   +      $current_theme['description'] = [
   +        '#theme' => 'details',
   +        '#title' => t($theme->info['description']),
   +        '#description' => $description,
   +      ];
   

   please s/$description/$details_description on next re-roll

Updated patch, please review

+1 rtbc, leaving for mark-up and css review

following failures

exception: [Notice] Line 246 of core/includes/form.inc:
Undefined index: #attributes

+++ b/core/includes/form.inc
@@ -243,7 +243,7 @@ function template_preprocess_fieldset(&$variables) {
-  $variables['attributes'] = $element['#attributes'];
+  $variables['attributes'] = isset($element['#attributes']) ? $element['#attributes'] : new Attribute();

looks that still needed

I think that hopefully this will fix the failure.

yay! green now needs frontender's eyes on it

Applying the same pattern as on the extend page seems like a good idea. Not sure if the last usability test showed any problems with this pattern on the extend page.

In reviewing this on simplytest I noticed two things:

- It's actually quite handy to have the actionable links outside of the collapsed information like on the appearance page, where they are hidden on the extend page. This is definately out of scope for this issue of course.
- The description font size on appearance is smaller than the same on the extend page . Would be nice to correct that (remove the 0.95em sizing), all those small font-size differences do not look too good.

Reroll of #50, fixing the .95em sizing by getting rid of the description class as suggested in #29-3.

Also aligned the templates for both classy and seven.

Reviewed patch #53, it seems ready for RTBC to me, but I don't change the status since it's my first patch review :-D

Reviewed patch #53 for second time, this time from drupalcon BCN ;-)
Everything ok; attached screenshot with before-after states.

Thanks for the screengrab, it looks great to me.

The only problem I have with this visually is the focus on the details element. There is an issue to remove these focus effects completely: #2489450: Remove unnecessary focused/hover effects on details and vertical tabs

Thanks for the work here making the module page CSS reusable!

Hmm, perhaps this should be back to Needs work untill the other issue is fixed?

I think we can just have it as a follow up issue.

This conflicts with #2151109: Convert theme_system_modules_details() to Twig could use cross-review.

We can work on this again after that one gets committed.

Re-rolling following #2151109: Convert theme_system_modules_details() to Twig.

It makes no sense to use the system_extension_details templates in light of the that issue, so this patch copies part of the twig template from there.

Great, the appearance is as the screenshots in #62.

Thank you!

Can we please make sure #2489450: Remove unnecessary focused/hover effects on details and vertical tabs gets adequate follow up, I am always a bit cautious when we do add stuff - but not have anyone of that thread active in the following issues it creates.

@jcnventura Could you make sure we appropriately follow up.

Nothing to really hold up RTBC but one small point and a question.

1. +++ b/core/modules/system/system.admin.inc
   @@ -341,6 +341,18 @@ function template_preprocess_system_themes_page(&$variables) {
   +  // Build a list of theme names for all known themes
   

   Minor: Should end with a period per https://www.drupal.org/node/1354#general.

2. +++ b/core/modules/system/templates/system-themes-page.html.twig
   @@ -56,12 +56,29 @@
   +                  <div class="admin-requirements">{{ 'Machine name: @module-machine-name'|t({'@module-machine-name': theme.machine_name}) }}</div>
   ...
   +                    <div class="admin-requirements">{{ 'Version: @module-version'|t({'@module-version': theme.version}) }}</div>
   ...
   +                    <div class="admin-requirements">{{ 'Requires: @module-list'|t({'@module-list': theme.requires|safe_join(', ')}) }}</div>
   ...
   +                    <div class="admin-requirements">{{ 'Required by: @module-list'|t({'@module-list': theme.required_by|safe_join(', ')}) }}</div>
   

   Just wondering if it would make sense for these place holders to be @theme-machine-name, @theme-version, @theme-list, etc.?

@Cottser: It would make sense for those placeholders to be @extension.machine-name, @extension.version, etc.

As a translator, I don't want to make superfluous strings. So the solution is to alter these strings, both in this template and in the module page template to be a bit more generic. I can certainly create a followup issue should this one get committed. But I'm not sure if it is possible to change strings post-RC1.

Good point, I think this is probably fine as is then even if it's not 100% accurate.

Here's a patch for #2489450: Remove unnecessary focused/hover effects on details and vertical tabs

And a really minor diff to address the #71.1 (lack of a final dot in the comment). For obvious reasons, not changing the RTBC status.

+1 :)

IS defines this as UX bug. Please change the IS if you think this is a task

This is a task. A UX bug indicates we have some actual user feedback that it actually causes significant issues, we have no data/feedback of such effect. In general we are quite careful with trowing around the bug indicator.

Thanks for the clarification :)

This patch only generalizes the system modules / theme pages CSS, and adds some documentation to the theme dependencies.

This is not rc eligible. If you want committers to consider it for inclusion in RC then the issue summary needs to include a statement of why we need to make this change and be tagged with rc target triage. It is possible that contrib or custom is already relying on the behaviour in HEAD.

I don't see anything changed in the issue summary to reflect alexpott his concerns.

Still don't think this should go in until #2489450: Remove unnecessary focused/hover effects on details and vertical tabs goes in.

I didn't change the issue summary, and I marked it RTBC again, because the 'needs work' was all about arguing on getting this into 'rc eligible' status. It's a fight I'm not going to fight. This will go into Drupal 8.1 or Drupal 9.

Part of our community dynamics, is that when a committer pushes it back from RTBC. We take the feedback and discuss it, even if at the end we decide that his/her arguments are moot. The request from alex in this process, is one that is common from a committer and requires the set status.

It is perfectly fine if your not up for it, but please be considerate of our community process - which is responding to feedback, especially from a committer and respecting his push back on status to reflect this.

I did consider it. He asked if I could justify the need to add this change now that we're in the RC stage. I cannot. Maybe someone else can. The fact that this is no longer considered a bug, and is considered a task, makes it somewhat harder to justify the reasoning on why this should be added at this stage. The 'work needed' is no longer in the patch, which is RTBC, but in justifying the need to get it considered as an RC target, since I don't think that needs to be done, I've backtracked to the state before I added the 'rc eligible' tag (#81).

I'll wait until 8.0.0 is released and set this back to RTBC then, unless someone decides to justify getting this into 8.0.0. If someone does, don't forget to change the version from 8.1.x to 8.0.x.

Setting back to RTBC as it was in #81, as the 'needs work' between #83 and now was all about justifying the need to have this go in to 8.0.0. That is no longer applicable, as the flux capacitor in my DeLorean is broken.

First of all, I am fixing a missing </div>. This won't make the tests pass. I will try to fix the tests in the next patch. I will reason why I am doing so in that comment.

This should fix the tests. I am not sure if this is acceptable as it changes markup in stable theme. The reason the tests failed was because the changes got overridden by stable.

Now, I am not sure why this twig file is in stable. It is not a template that is likely to be overridden by themers. Even if it is, there are no changes to classes, etc... but there are new elements. This likely needs maintainer review and feedback. If we can't have these changes, then I can't see any other way except that this issue goes to D9.

#92, that's a great idea. I didn't think of it. If we put this into seven, it would make sense as well as not break stable. Of course, other admin themes still may override but at least they will not break.

Sorry, I meant to say #94 earlier.

This patch reverts the change to stable's template and copies it to seven. I am not sure if the tests use seven when testing, but let's see.

I guess we are not using seven, or there is something wrong with the patch in #96. Any reviews please?

Now, I am not sure why this twig file is in stable.

Just to answer that question, Stable contains all core template files. Most of the files are in fact identical. Stable's and Classy's files cannot change, but the core file can. I only glanced at that test code, but it may have Stark on as the default theme, which means inheriting its template from Stable. I would manually verify that the change in this patch works with Stark and Classy as the admin theme, which I suspect it may not.