Posted by c4rl on January 25, 2013 at 5:57am
10 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | theme system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | Needs manual testing, Twig |
Issue Summary
Issue #1898478 by likin, killerpoke, EVIIILJ, vlad.dancer, podarok, mh86, steveoliver | c4rl: Convert menu.inc to Twig.Task
Use Twig instead of PHPTemplate
Remaining
- Patch needs review
- Manual testing
| Theme function name/template path | Conversion status |
|---|---|
| theme_menu_link | converted |
| theme_menu_local_action | converted |
| theme_menu_local_task | converted |
| theme_menu_local_tasks | converted |
| theme_menu_tree | converted |
| bartik_menu_tree | converted here instead of #1938840: bartik.theme - Convert PHPTemplate templates to Twig for dependency reasons |
Related
#1757550: [meta] Convert core PHPTemplate files and theme functions to Twig
Comments
#1
Tagging
#2
Taking this on. Going for consolidation/use of other things like theme('item_list'), in the process.
#3
Consolidated various issues* from the Twig sandbox into this one patch.
Not consolidating or using item_list as mentioned in #2, for two reasons: 1. Straight conversion; 2. These don't always have to be formatted as lists.
TODO (once #1905584: Move base theme system templates into /core/templates is fixed):
theme_*functions from menu.inc./core/themes/stark/templates/menu.inc/*to/core/includes/templates/menu.inc/*Git attribution goes to: likin, killerpoke, EVIIILJ, vlad.dancer, podarok, mh86, steveoliver
Previous sandbox issues, where these came from, in part:
#1843564: Convert theme('menu_local_tasks') to Twig
#1843560: Convert theme('menu_local_task') to Twig
#1824618: Convert theme('menu_local_action') to Twig
#4
The last submitted patch, drupal-twig-menu.inc--1898478-3.patch, failed testing.
#5
+++ b/core/themes/stark/templates/menu.inc/menu-local-tasks.html.twig@@ -0,0 +1,26 @@
+* @ingroup themeable
+*/
+#}
+{% if primary_tasks %}
+ <h2 class="element-invisible">{{ 'Primary tabs'|t }}</h2>
+ <ul class="tabs primary">{{ primary_tasks }}</ul>
+{% endif %}
+{% if secondary_tasks %}
+ <h2 class="element-invisible">{{ 'Secondary tabs'|t }}</h2>
+ <ul class="tabs secondary">{{ secondary_tasks }}</ul>
Please can we follow BEM principles with our class names. Just adding a class of 'primary' or 'secondary' can and will cause conflicts. Please use 'tabs--primary' and 'tabs--secondary' instead.
Here is a brief explanation of BEM for reference.
BEM is a methodology for naming
and classifying CSS selectors in a way to make them a lot more strict,
transparent and informative.
The naming convention follows this pattern:
.block{}.block__element{}
.block--modifier{}
* `.block` represents the higher level of an abstraction or component.
* `.block__element` represents a descendent of `.block` that helps form `.block` as a whole.
* `.block--modifier` represents a different state or version of `.block`.
An **analogy** of how BEM classes work might be:
.person{}.person--woman{}
.person__hand{}
.person__hand--left{}
.person__hand--right{}
#6
@mbrett5062 - Thanks for the feedback!
We're working hard to get these Twig templates into core, there will definitely be more cleanup afterwards but for now we're focused on getting these templates into core before feature freeze.
There have been recent discussions and work towards CSS coding standards:
g.d.o announcement and discussion:
http://groups.drupal.org/node/277223
Draft coding standards:
http://drupal.org/node/1886770 and more specifically http://drupal.org/node/1887918 which relates to architecture.
Also #1900768: Consider BEM for CSS Coding Standards in Drupal 8.
Once the standards are worked out, the task of refactoring CSS to meet these standards would probably be best done with a meta issue and sub issues, similar to #1190252: Meta: Use csslint as a weapon to beat the crappy CSS out of Drupal core. I wasn't able to find an existing meta issue tracking this.
#7
Thanks for the feedback @cottser, and I agree with doing these changes in a follow up. Just glad it is noted.
#8
Just a head's up: #916388: Convert menu links into entities is a few days from landing. We'll probably want to wait till then before looking at this again.
#9
Based on #1905584: Move base theme system templates into /core/templates it seems this issue would be accommodated by #1898454: [READY] system.module - Convert PHPTemplate templates to Twig, though that may result in a large patch.
#10
Based on the state of #1905584: Move base theme system templates into /core/templates which introduces support for /core/templates for these includes, this issue is active and I'm on it...
#11
Ignore what I said about system.module in #9, we'll proceed to convert this separately of system.module and put in /core/templates as described in #1905584: Move base theme system templates into /core/templates
#12
#3: drupal-twig-menu.inc--1898478-3.patch queued for re-testing.
#13
The last submitted patch, drupal-twig-menu.inc--1898478-3.patch, failed testing.
#14
We need to update drupal_common_theme() in theme.inc to add 'template' indexes for all these new templates.
#15
Working on a new patch right now to move this along a bit.
#16
Summary of changes:
The interdiff is rolled with
git diff -C -Mto account for the file moves.All the failing tests pass locally so hopefully testbot agrees :)
#17
Wrong interdiff, here's the smaller one.
#18
The last submitted patch, 1898478-16.patch, failed testing.
#19
+++ b/core/includes/menu.incundefined@@ -1555,106 +1555,92 @@ function _menu_tree_data(&$links, $parents, $depth) {
- 'localized_options' => array(),
+ 'localized_options' => new Attribute(),
Uh, oops. That doesn't belong there.
#20
This simplifies template_preprocess_menu_link() (removes drupal_render() and ternary) and updates one of the preprocess docblocks to match #1913208: [policy] Standardize template preprocess function documentation.
#21
Missed "Default template". Interdiff is from #19.
#22
One more, sorry for the barrage. Just another docs tweak, interdiff is from #19 again.
#23
Tagging.
#24
Unassigning so this can be reviewed.
#25
Review for #22:
+ $variables['primary_tasks'] = (!empty($variables['primary'])) ? drupal_render($variables['primary']) : FALSE;+ $variables['secondary_tasks'] = (!empty($variables['secondary'])) ? drupal_render($variables['secondary']) : FALSE;
Do we need to render these in the preprocess? they should just work as-is in the template, right? In fact, do we even need the preprocess at all here? looks like we could just use {{ primary }} and {{ secondary }} in the template.
We have a new template for menu_local_tasks but the theme function wasn't removed in the patch.
/*** Markup generated by theme_menu_local_tasks().
*/
.tabs > li {
margin-left: 0.3em;
margin-right: 0;
}
This still exists in system.theme-rtl.css
/*** Markup generated by theme_menu_local_tasks().
*/
div.tabs {
margin: 1em 0;
}
This still exists in system.theme.css
#26
Cleaned up some docs and removed the preprocessing and theme for menu_local_tasks from #25
#27
bartik_menu_tree() needs to be converted here too. If the wrapper in menu_tree.html.twig is changed to use an Attribute(), then bartik can just implement a preprocess, since all it's doing is adding a class.
#28
Did #27, thanks @duellj I went with the twig file as discussed on IRC to help with #1854344: [meta] Goals for core themes: Make Stark as semantic as possible; Make Bartik a better prospective base theme
#29
Per #1757550-44: [meta] Convert core PHPTemplate files and theme functions to Twig, retitling to indicate this issue applies to theme_ functions, which are lower in priority than PHPTemplate conversion issues.
#30
#31
Menu's, submenu's, breadcrumbs and action links are ok, they have the same html output in both Stark and Bartik.
Tabs need a little more work.
The output for the tabs isn't the same (in both Stark and Bartik). On the active tab,
<span class="element-invisible">(active tab)</span>should be a child of the link, but in the twig version, it is a sibling.