Problem/Motivation

As part of #3110855: Plan for removing dependency to Stable in Bartik/Seven/Claro/Umami, core themes should not require anything in stable.theme

Proposed resolution

Identify any regressions that would occur without stable.theme and address them in the core themes.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#7 3117217-7.patch30.61 KBbnjmnm
#4 3117217-4.patch26.1 KBbnjmnm
#4 interdiff_2-4.txt3.15 KBbnjmnm
#4 3117217-4-MANUAL-TESTING-do-not-test.patch28.53 KBbnjmnm
#2 3117217-2.patch27.02 KBbnjmnm
#2 3117217-2_MANUAL-TESTING-do-not-test.patch29.45 KBbnjmnm

Comments

bnjmnm created an issue. See original summary.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Assigned: bnjmnm » Unassigned
Status: Active » Needs review
StatusFileSize
new 3117217-2_MANUAL-TESTING-do-not-test.patch29.45 KB
new 3117217-2.patch27.02 KB

Evaluation of the five functions in stable.theme

☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️

stable_preprocess_links()
There was already agreement in #3097890: Remove all @deprecated code in core themes that this was unnecessary for Drupal 9. It was not removed because it should remain in Stable. It will not be part of Stable 9 and is not necessary for core themes.

☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️

stable_preprocess_image_widget()
Added in #3033279: Move markup from template_preprocess_image_widget to a template. This adds file size to file-link templates inside of image widgets. That issue mistakenly added file_size whenever present to core's file-link template, something that is also occurring in Claro, which prompted this followup: #3117430: file-link template should not always display file_size.

The approach of stable_preprocess_image_widget() is not ideal (adding markup in #suffix), but a more elegant solution is not straightforward enough to be in-scope for this issue. Unfortunately, this means copying this not-perfect approach to Umami, Bartik and Seven (with a @todo for the issue aiming to find a better solution). @lauriii agreed that the more elegant solution is best applied in a different issue.

☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️☠️

stable_library_info_alter()
stable_element_info_alter()
stable_process_text_format()

These three were added in #2881212: Formatting guidelines toggle script relies on Classy selectors. This issue removed CSS styling and class additions from filter.module that should have been included in the theme layer. These functions were added to stable.theme so the changes in filter.module didn't break BC.

I opted for an approach that did not use functions in the .theme file.
- The class additions are now done in new templates for filter-wrapper, filter-guidelines, and filter-help (note that the .filter-list class added in Stable did not need to be addressed as this class isn't targeted anywhere).
- A new /filter library was added to each theme, and configured to extend both of core's filter libraries. In Seven, Bartik and Umami, a copy of filter.admin.css is used by this library. Claro uses the CSS that was previously overriding Stable's copy.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

+1 for cleaning this up in #3117430: file-link template should not always display file_size.

+++ b/core/profiles/demo_umami/themes/umami/umami.theme
@@ -140,3 +140,20 @@ function umami_form_alter(array &$form, FormStateInterface $form_state, $form_id
+    if (!isset($variables['data'][$file_key]['filename']['#suffix']) || strpos($variables['data'][$file_key]['filename']['#suffix'], '<span class="file-size">') === FALSE) {

+++ b/core/themes/bartik/bartik.theme
@@ -165,3 +165,20 @@ function bartik_preprocess_links__media_library_menu(array &$variables) {
+    if (!isset($variables['data'][$file_key]['filename']['#suffix']) || strpos($variables['data'][$file_key]['filename']['#suffix'], '<span class="file-size">') === FALSE) {

+++ b/core/themes/seven/seven.theme
@@ -424,6 +424,18 @@ function seven_preprocess_image_widget(array &$variables) {
+    if (!isset($data[$file_key]['filename']['#suffix']) || strpos($data[$file_key]['filename']['#suffix'], '<span class="file-size">') === FALSE) {

Why do we have to check if the suffix has been already set?

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
StatusFileSize
new 3117217-4-MANUAL-TESTING-do-not-test.patch28.53 KB
new interdiff_2-4.txt3.15 KB
new 3117217-4.patch26.1 KB

#3 right, it's unnecessary to check the suffix since it's being overwritten, not appended.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Reviewed & tested by the community

Tested manually on a node form with all themes (Umami, Bartik, Seven, Claro). Both, formatted text and image / file upload widgets seem to work as expected. I also double checked that all themes have template override for file-link.html.twig to make sure that the filesize isn't rendered twice.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

As an additional assurance, I tested this for regressions with Wraith with all four core themes. I checked admin/content (for the preprocess_links() removal) and with a node edit page that covers the other elements impacted in this issue: an already uploaded image that displays file size and a body field with text filter. No differences were spotted.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
StatusFileSize
new 3117217-7.patch30.61 KB

Here's an attempt to make the choices in this patch more easily understood.

First, I've attached a version the patch from #4, but created using diff -C -C origin/9.0.x ... instead of just diff origin/9.0.x .... This makes it clearer that filter.admin.css was copied from Stable to Bartik, Umami, and Seven, with the only changes being image paths. However, this also results in a patch that reports that the new container--text-format-filter(something).html.twig templates are copied from media library. This isn't the case -- they are just quite similar.

Here's a summary of what was done for each function that needed functionality replicated in core themes:

Stable function How it was moved to core themes
stable_library_info_alter() adds the file filter.admin.css to two libraries: filter/drupal.filter.admin, and filter/drupal.filtern
  • Claro:. Did not need to copy filter.admin.css as it has its own filter styling. However, it was necessary to change how Claro loaded its filter styling. Previously, it did so by overriding the request for Stable's filter.admin.css. Since that asset will no longer be loaded, Claro now loads filter.admin.css via a newly-created claro/filter library, which loads as an extension of core's filter libraries <code>filter/drupal.filter.admin: - claro/filter filter/drupal.filter: - claro/filter
  • Bartik, Umami, Seven: needed their own copies of filter.admin.css, which is loaded via a new (themename)/filter library that load as an extension of filter/drupal.filter.admin and filter/drupal.filter
stable_element_info_alter() adds stable_process_text_format() as a #process callback for filter elements. This callback adds classes to several parts of the filter. Instead of adding via two functions in a .theme file, they are now added via dedicated templates (simlar to how media library does it via container--media-library-content. TextFormat.php was updated so the text format element recognizes three new templates: container--text-format-filter-wrapper, container--text-format-filter-help, container--text-format-filter-guidelines. These three templates are where the elements get the classes .filter-wrapper, .filter-help, .filter-guidelines
stable_preprocess_image_widget() adds file size markup to the file-link element in the render array #suffix
  • Claro: Not needed, Claro displays file size via the file-link template, not a #suffix in the render array.
  • Bartik, Umami, Seven:. Copied this functionality to their respective .theme files.
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

I've looked for differences in the theme out but can't find any. This looks good and the next step on the path towards decouple all the themes from stable.

Committed 5c5ca6d and pushed to 9.0.x. Thanks!

  • alexpott committed 5c5ca6d on 9.0.x 
    Issue #3117217 by bnjmnm, lauriii: Decouple core theme dependency on...

Status: Fixed » Closed (fixed)

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