Posted by c4rl on January 25, 2013 at 5:43am
10 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | overlay.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | fixed |
| Issue tags: | Twig |
Issue Summary
Issue #1898436 by chrisjlee, joelpittet, gnuget, Cottser: overlay.module - Convert PHPTemplate templates to Twig.Task
Convert PHPTemplate templates to Twig templates.
Remaining
n/a
| Template path | Conversion status |
|---|---|
| core/modules/overlay/templates/overlay.tpl.php | converted |
Profiling results
=== 8.x..8.x compared (51981beb768ad..51981e001fd46):
ct : 36,140|36,140|0|0.0%
wt : 345,481|345,338|-143|-0.0%
cpu : 316,694|316,391|-303|-0.1%
mu : 28,878,264|28,878,264|0|0.0%
pmu : 28,993,328|28,993,328|0|0.0%http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51981beb768ad&...
=== 8.x..overlay-1898436-33 compared (51981beb768ad..51981e3ae1e6c):
ct : 36,140|36,220|80|0.2%
wt : 345,481|345,604|123|0.0%
cpu : 316,694|317,226|532|0.2%
mu : 28,878,264|28,913,256|34,992|0.1%
pmu : 28,993,328|29,022,712|29,384|0.1%http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51981beb768ad&...
Related
#1987408: overlay.module - Convert theme_ functions to Twig
#1757550: [meta] Convert core PHPTemplate files and theme functions to Twig
Comments
#1
Tagging
#2
#3
not sure if i should add back in
template_preprocess_overlay_disable_messagefunction. if so #4 includes adding back in that function to core.#4
This patch includes the
template_preprocess_overlay_disable_messagefunction.#5
Patch works as expected when applied to core. Marked as reviewed but should probably use classes instead of ids.
#6
Awesome!!
I would love a quick sign-off on this from one of the Twigluminati ;), so arbitrarily assigning to c4rl since he was up above.
#7
@zakiya Totally agree, we should get rid of the ID, although not in this issue, this is meant to be a as straight forward conversion as possible. Could you open another issue as maybe a follow-up to this one to look at removing the ID and fixing the JS/CSS that that change may affect?
+ $variables['profile_link'] = drupal_render($element['profile_link']);+ $variables['dismiss_link'] = drupal_render($element['dismiss_message_link']);
Removed the
drupal_render()And removed
theme_overlay_disable_message()because from what I remember(unless things have changed) we are getting rid of these.And just some spacing nitpicks are in here too.
Functionally I would give this an RTBC.
#8
Found some documentation tweaks that can be done to move this along further, and tagging for manual testing.
+++ b/core/modules/overlay/overlay.moduleundefined@@ -546,6 +510,36 @@ function template_preprocess_overlay(&$variables) {
+ * Preprocess variables for the message about how to disable the overlay.
+ *
+ * @param $variables
+ * An associative array with an 'element' element, which itself is an
+ * associative array containing:
+ * - profile_link: The link to this user's account.
+ * - dismiss_message_link: The link to dismiss the overlay.
+ *
+ * @ingroup themeable
This should be updated to match #1913208: [policy] Standardize template preprocess function documentation, something like:
Prepares variables for overlay disable message templates.
Default template: overlay-disable-message.html.twig.
@param array $variables
…and so on. No @ingroup needed.
+++ b/core/modules/overlay/overlay.moduleundefined@@ -546,6 +510,36 @@ function template_preprocess_overlay(&$variables) {
+ // Render the links.
This comment should be removed, since we're not rendering yet.
+++ b/core/modules/overlay/templates/overlay-disable-message.html.twigundefined@@ -0,0 +1,20 @@
+ * @file
+ * Default theme implementation for the message about how to disable the overlay.
If we change "how to disable" to "disabling" this will fit within the 80 character limit for summaries (http://drupal.org/node/1354#file).
+++ b/core/modules/overlay/templates/overlay-disable-message.html.twigundefined@@ -0,0 +1,20 @@
+ * - attributes: An array of element attributes.
We can probably remove this line, the attributes are not output in the template. If we keep it, we should say something like "HTML attributes for the containing element.", assuming the attributes are for the
<div>.+++ b/core/modules/overlay/templates/overlay.html.twigundefined@@ -0,0 +1,34 @@
+ * - tabs: Tabs linking to any sub-pages beneath the current page (e.g., the view
+ * and edit tabs when displaying a node).
First line is 81 characters, should wrap to 80 per http://drupal.org/node/1354#drupal. The second line should match the indent of the first, lining up with the 't' in 'tabs'.
#9
Got the okay from @joelpittet to unassign.
#10
Creating a new patch and interdiff based on #7 with the documentation changes in #8 would be a good novice task.
#11
Attempt to reroll.
@cottser Thanks for the helpful and thorough review
#12
oops forgot to remove @themeable
#13
Changes as per conversation with @Cottser
- Remove line above where @themeable in docblock was
#14
Realized i forgot to wrap one of the docblock lines as per comment #8.5.
#15
Thanks for keeping this moving @chrisjlee! I just tested this patch manually and found that overlay-disable-message.html.twig was not being picked up, we need to add the 'template' declaration in overlay_theme() for that template to be used.
And some more documentation tweaks:
+++ b/core/modules/overlay/templates/overlay-disable-message.html.twigundefined@@ -0,0 +1,19 @@
+ * - profile_link: Rendered link.
+ * - dismiss_link: Rendered link to disable the message.
In overlay-disable-message.html.twig, let's use the same documentation for these variables as in template_preprocess_overlay_disable_message(), the preprocess function docs are much better.
+++ b/core/modules/overlay/templates/overlay.html.twigundefined@@ -0,0 +1,34 @@
+ * - title: the (sanitized) title of the page.
In overlay.html.twig, the description of the 'title' variable should start with a capital letter (capitalize the first 'the') per http://drupal.org/node/1354#drupal.
#16
@Cottser thanks for the feedback again it was very helpful and thorough.
#17
Looks good, just needs this docs tweak from #15 then I think this one is ready! I tested this one manually, works much better now :)
#18
The above change is a good novice task, tagging. Once that's done this patch is RTBC in my opinion.
If you'd like to work on this, please assign the issue to yourself. If you get stuck, drop by #drupal-twig on IRC or post your questions here. Thanks!
#19
Overlay works better for me as well after the patch.
Changes as per #15
#20
#21
I've been eyeing the first hunk of this patch and I finally tracked it down to being a reversal of part of http://drupalcode.org/project/drupal.git/commit/b574b9e. @chrisjlee is on it!
#22
remove offending change reference according to #21
#23
Ah, much better. This is good to go when it comes back green, thanks for all your work on this @chrisjlee!
#24
The last submitted patch, 1898436-convert-overlay-to-twig-22.patch, failed testing.
#25
#22: 1898436-convert-overlay-to-twig-22.patch queued for re-testing.
#26
Testbot decided it didn't want to apply the patch, back to RTBC :)
#27
Per #1757550-44: [meta] Convert core PHPTemplate files and theme functions to Twig, retitling to indicate this issue applies to templates rather than theme_ functions. See #1987408: overlay.module - Convert theme_ functions to Twig for theme_ function conversion.
#28
#29
Adding a patch only with the .tpl.php conversions.
#30
Thanks @gnuget! We'll need a patch with only the theme_ function conversions uploaded at #1987408: overlay.module - Convert theme_ functions to Twig to complete the patch split. One way of doing this is by applying #22 locally then reverse applying #29 (
git apply -R patchname.patch).#31
Since gnuget assigned it to anonymous, I took care of #30. Let me know if there are any issues.
#32
+++ b/core/modules/overlay/templates/overlay.html.twigundefined@@ -0,0 +1,34 @@
+ <h2 class="element-invisible">{{ 'Primary tabs'|t }}</h2><ul id="overlay-tabs">{{ render_var(tabs) }}</ul>
We should be able to lose the render_var here, just {{ tabs }} please.
#33
Removing the render_var
#34
Tagging for profiling.
#35
I'll be profiling this and giving it another review.
#36
Profiling results look great.
=== 8.x..8.x compared (51981beb768ad..51981e001fd46):
ct : 36,140|36,140|0|0.0%
wt : 345,481|345,338|-143|-0.0%
cpu : 316,694|316,391|-303|-0.1%
mu : 28,878,264|28,878,264|0|0.0%
pmu : 28,993,328|28,993,328|0|0.0%
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51981beb768ad&...
=== 8.x..overlay-1898436-33 compared (51981beb768ad..51981e3ae1e6c):
ct : 36,140|36,220|80|0.2%
wt : 345,481|345,604|123|0.0%
cpu : 316,694|317,226|532|0.2%
mu : 28,878,264|28,913,256|34,992|0.1%
pmu : 28,993,328|29,022,712|29,384|0.1%
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51981beb768ad&...
This needs a few small tweaks and then it's RTBC from my perspective:
+++ b/core/modules/overlay/templates/overlay.html.twigundefined
@@ -0,0 +1,34 @@
+ <a id="overlay-close" href="#" class="overlay-close"><span class="element-invisible">{{ 'Close overlay'|t }}</span></a>
+++ /dev/nullundefined
@@ -1,38 +0,0 @@
- <a id="overlay-close" href="#" class="overlay-close" role="button" aria-controls="overlay-content"><span class="element-invisible"><?php print t('Close overlay'); ?></span></a>
We need to add the 'role' and 'aria-controls' attributes as in the .tpl.php version.
+++ b/core/modules/overlay/templates/overlay.html.twigundefined@@ -0,0 +1,34 @@
+{{ disable_overlay }}
+<div id="overlay"{{ attributes }}>
...
+ <h1 id="overlay-title"{{ title_attributes }}>{{ title }}</h1>
...
+ <div id="overlay-content"{{ content_attributes }}>
We should document at least 'disable_overlay' - something like "the message about how to disable the overlay."?, but we can probably document the attributes ones too by copying and pasting from existing conversions.
#37
Changes from #36
#38
few more things, moved ids to the preprocess and added attributes comments.
#39
Wow, even better. Thanks @joelpittet!
Just rolling in a typo fix for pre-existing docs. RTBC :)
#40
The last submitted patch, 1898436-39.patch, failed testing.
#41
#39: 1898436-39.patch queued for re-testing.
#42
Looks good, back to rtbc.
#43
+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
#44
Committed b171982 and pushed to 8.x. Thanks!