Support from Acquia helps fund testing for Drupal Acquia logo

Comments

psynaptic’s picture

Wrong link in original post:

http://drupalcontrib.org/api/drupal/drupal--modules--filter--filter.modu...

My question would be, how would we override this if it was a hardcoded link?

psynaptic’s picture

Ahh, this is used in core as:

$element['format']['help'] = array(
  '#type' => 'container',
  '#theme' => 'filter_tips_more_info',
  '#attributes' => array('class' => array('filter-help')),
  '#weight' => 0,
);

So we can just change the theme hook to link and use a pattern like link__filter_tips_more_info. This would allow the link to be overridden in this one specific place if needed.

jessebeach’s picture

@psynaptic, that seems the most backwards-compatible approach that gets us to the goal of standardization in the default case.

droplet’s picture

In case, if we remove theme_filter_tips_more_info, we also remove its in render element. That seems the best practice. Render Arrays isn't a substitute for theme function. But like to see a patch to show how it being removed.

is it related to html5 ?

helior’s picture

Issue tags: -html5

Removing html5 tag.

aspilicious’s picture

Issue tags: +html5

This is part of the html5 initiative.

RobLoach’s picture

The whole filter tip situation here is really gross. The theme function is difficult to work with, and the markup itself that is generated is pretty ugly. Switching it to a renderable array as droplet suggested is a great start.

Jacine’s picture

Component: theme system » markup
Issue tags: -html5 +theme system cleanup

In an effort to get a better picture of issues remaining in the HTML5 Initiative, we are removing the "html5" tag from issues that are not directly HTML5-related. Tagging this "theme system cleanup" instead, as that's more accurate. Note: Any issues assigned to the "Markup, CSS and JavaScript" components will still be broadcast on the HTML5 Twitter feed so that interested parties are aware and can participate.

jenlampton’s picture

Issue tags: +Twig

Also tagging for Twig since removing this theme function means one less thing to convert.

sun’s picture

Component: markup » filter.module
Status: Active » Needs review
Issue tags: +Theme Component Library
FileSize
1.78 KB

Fixed the issue summary.

Closely related: #1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays

The shared problem space among these is that the link being output intends to be a block element. In this case here, it is wrapped in a paragraph. In aforementioned issue, it is wrapped in a DIV.

Thus, I wonder whether the approach in attached patch would be an acceptable solution for these kind of links?

Essentially this addition to system.theme.css:

/**
 * Faux block elements.
 */
a.block {
  display: block;
}

What do you think?

sun’s picture

Title: theme_filter_tips_more_info() should be replaced with the l() function » Replace theme_filter_tips_more_info() with theme_link()

Adjusting title.

Status: Needs review » Needs work
Issue tags: -Twig, -theme system cleanup, -Theme Component Library

The last submitted patch, drupal8.theme-filter-tips-more-info.10.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +Twig, +theme system cleanup, +Theme Component Library

#10: drupal8.theme-filter-tips-more-info.10.patch queued for re-testing.

EDIT This test sometimes fails ...

Drupal\locale\Tests\LocaleFileImportStatus
File is updated.	Other	LocaleFileImportStatus.php	159	Drupal\locale\Tests\LocaleFileImportStatus->testBulkImportUpdateExisting()
andypost’s picture

+++ b/core/modules/system/system.theme.css
@@ -38,6 +38,13 @@ tr.odd {
+ * Faux block elements.
+ */
+a.block {
+  display: block;

I think we need better(quicker) selector here

jenlampton’s picture

Title: Remove theme_filter_tips_more_info() from core » Replace theme_filter_tips_more_info() with l()

Since theme_filter_tips_more_info currently uses the l() function to generate it's markup. We should remove this theme function, the use of #type = container, and the use of #theme, and instead call l() directly for the form element. If this needs to be altered we have hook_form_alter. Let's not over complicate things.

  $element['format']['help'] = array(
    '#type' => 'markup',
    '#value' => l(t('More information about text formats'), 'filter/tips', array('attributes' => array('class' => array('filter-help'), 'target' => '_blank')));
    '#weight' => 0,
  );

see #1833906: Remove theme_link() from core
see #1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays

jenlampton’s picture

Title: Replace theme_filter_tips_more_info() with theme_link() » Remove theme_filter_tips_more_info() from core
FileSize
1.38 KB

patchy

Title: Replace theme_filter_tips_more_info() with l() » Remove theme_filter_tips_more_info() from core
Status: Needs review » Needs work

The last submitted patch, core-remove_theme_filter_tips_more_info-1595614-16.patch, failed testing.

sun’s picture

+++ b/core/modules/filter/filter.module
@@ -1004,9 +1001,7 @@ function filter_process_format($element) {
   $element['format']['help'] = array(
-    '#type' => 'container',
-    '#theme' => 'filter_tips_more_info',
-    '#attributes' => array('class' => array('filter-help')),
+    '#markup' => '<div class="filter-help">' . l(t('More information about text formats'), 'filter/tips', array('attributes' => array('target' => '_blank'))) . '</div>',
     '#weight' => 0,
   );

Can we keep the #type container, just remove #theme, and turn the value of #markup just into the l()?

Fabianx’s picture

Agree with #18

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
1.33 KB
724 bytes

Re-rolled to catch up with HEAD and made changes requested in #18

andypost’s picture

Status: Needs review » Reviewed & tested by the community

nice clean-up

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.