I don't know if these issues titles make enough sense or not, but I'm thinking to label them as their theme api interface. Anyways...

1. theme('toolbar') is a little messed up (as in messy and not even working) and
2. theme('toolbar_toggle') needs to be replaced with theme('link') as per #1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays.

CommentFileSizeAuthor
#15 Screen Shot 2013-01-26 at 4.12.49 PM.png24.17 KBjohnnydarkko
#11 d8tts-theme-toolbar-1825828-11.patch2.05 KBsteveoliver
#7 d8tts-theme-toolbar-1825828-7.patch7.58 KBsteveoliver
#3 d8tts-theme-toolbar-1825828-3.patch8.19 KBsteveoliver
#1 d8tts-theme-toolbar-1825828-1.patch8.34 KBsteveoliver

Comments

steveoliver’s picture

steveoliver commented
Status: Needs work » Needs review
StatusFileSize
new d8tts-theme-toolbar-1825828-1.patch8.34 KB

This is how I think theme('toolbar') should work.

1. Uses theme('link') instead of theme('toolbar_toggle') (Recommended in #1779104: Convert theme_toolbar_toggle to twig (#30).
2. Cleans up template by implementing template_preprocess_toolbar.

fabianx’s picture

fabianx commented
Status: Needs review » Needs work

Do not pre-render things before the template.

Render is called explicitly, so {{ toolbar.toolbar_home }} will work.

steveoliver’s picture

steveoliver commented
Status: Needs work » Needs review
StatusFileSize
new d8tts-theme-toolbar-1825828-3.patch8.19 KB

Fabianx: Keeping preprocess, but not render() ing.

?

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented

#3 looks good for me
any backgrounds from FabianX ?

fabianx’s picture

fabianx commented
Status: Needs review » Needs work

Nope,

preprocess will be as much as possible removed even later.

Do not add additional vars.

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented 
+++ b/core/modules/toolbar/toolbar.moduleundefined
@@ -42,13 +44,6 @@ function toolbar_theme($existing, $type, $theme, $path) {
-  $items['toolbar_toggle'] = array(
-    'variables' => array(
-      'collapsed' => NULL,
-      'attributes' => array(),
-    ),
-    'template' => 'toolbar-toggle'

#5 is about this

steveoliver’s picture

steveoliver commented
StatusFileSize
new d8tts-theme-toolbar-1825828-7.patch7.58 KB

Without preprocess...

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented

#7 deletes toolbar_theme()
looks like wrong patch again

+++ b/core/modules/toolbar/toolbar.moduleundefined
@@ -42,13 +44,6 @@ function toolbar_theme($existing, $type, $theme, $path) {
-  $items['toolbar_toggle'] = array(
-    'variables' => array(
-      'collapsed' => NULL,
-      'attributes' => array(),
-    ),
-    'template' => 'toolbar-toggle'
-  );
steveoliver’s picture

steveoliver commented
Status: Needs work » Needs review

That's the point. It's a silly theme function I'm trying to replace with an implementation of theme('link').

steveoliver’s picture

steveoliver commented

Also remember this needs #1825820: theme('link')

steveoliver’s picture

steveoliver commented
StatusFileSize
new d8tts-theme-toolbar-1825828-11.patch2.05 KB

1. Toolbar toggle link can use theme('link') without preprocess and without theme('toolbar_toggle')) in #1779104-37: Convert theme_toolbar_toggle to twig.
3. This attached patch cleans up the toolbar.html.twig template.

steveoliver’s picture

steveoliver commented
Status: Needs review » Fixed
Issue tags: +theme system cleanup

This is just cleanup. Committed in bc0fe9e.

jessebeach’s picture

jessebeach commented

Hi all, I just wanted to mention that the toolbar theme issues are being addressed in the responsive toolbar patch. I just switched from tpl to twig in the latest patch.

#1137920-282: Fix toolbar on small screen sizes and redesign toolbar for desktop

I would suggest closing this issue as a duplicate or at least postponing it. I'd like to avoid you all spending time on unnecessary work here.

fabianx’s picture

fabianx commented
Status: Fixed » Closed (duplicate)
johnnydarkko’s picture

johnnydarkko commented
Component: Twig templates » Twig templates conversion (front-end branch)
Status: Closed (duplicate) » Needs work
StatusFileSize
new Screen Shot 2013-01-26 at 4.12.49 PM.png24.17 KB

Toolbar wasn't showing up.

Accidentally reopened and commented about this issue in #1779104: Convert theme_toolbar_toggle to twig sorry...

Committed WIP in 92bf9756

This is as far as I got:
Screen Shot 2013-01-26 at 4.12.49 PM.png

Project: » Lost & found issues

This issue’s project has disappeared. Most likely, it was a sandbox project, which can be deleted by its maintainer. See the Lost & found issues project page for more details. (The missing project ID was 1750250)