Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Task
Use Twig instead of PHPTemplate
Twig sandbox: http://drupal.org/sandbox/pixelmord/1750250
Remaining
- Manual testing
Testing steps
Install Drupal with JavaScript disabled. See #1757550-34: [Meta] Convert core theme functions to Twig templates for more tips on manual testing.
Related
Comment | File | Size | Author |
---|---|---|---|
#33 | twig-progress-bar-1939100-33.patch | 2.51 KB | oriol_e9g |
#26 | twig-progress-bar-1939100-26.patch | 2.54 KB | oriol_e9g |
#27 | twig-progress-bar-1939100-27.patch | 2.54 KB | oriol_e9g |
#22 | twig-convert_progress_bar-1939100-21.patch | 2.23 KB | jenlampton |
#21 | twig-convert_toolbar-1898464-61.patch | 8.88 KB | jenlampton |
Comments
Comment #1
chrisjlee CreditAttribution: chrisjlee commentedInitial patch. Don't really know what i'm doing but i'd thought i'd give it a good try.
Comment #2
chrisjlee CreditAttribution: chrisjlee commentedChange status.
Comment #3
star-szr@chrisjlee, thanks for getting this rolling!
The template file needs to be reworked similarly to #1939094-5: Convert theme_more_help_link() to Twig, the available variables in the Twig template are percent and message.
Also the template file docs should match up with Twig coding standards: http://drupal.org/node/1823416.
Comment #4
star-szrAnd tagging, this will need manual testing.
Comment #5
nikkubhai CreditAttribution: nikkubhai commentedThe installation fails with the falling message on Installation profile step :
/** * Returns HTML for a progress bar. * * Note that the core Batch API uses this only for non-JavaScript batch jobs. * * @param $variables * An associative array containing: * - percent: The percentage of the progress. * - message: A string containing information to be displayed. */
Comment #6
star-szrThis is already converted in the sandbox, so we should pull the Twig template over. There's no preprocess needed for this one.
The Twig template is located at core/themes/stark/templates/theme.inc/progress-bar.html.twig (download link) in the sandbox, but in core it should live at core/modules/system/templates/progress-bar.html.twig.
Edited to add:
This should be progress-bar instead of progress_bar.
Comment #7
chrisjlee CreditAttribution: chrisjlee commentedComment #8
chrisjlee CreditAttribution: chrisjlee commentedComment #10
chrisjlee CreditAttribution: chrisjlee commented#8: 1939100-Convert_theme_progress_bar-8.patch queued for re-testing.
Comment #11
star-szrThanks @chrisjlee. Please change the template filename to progress-bar.html.twig and update the 'template' key in theme.inc. Thanks!
I did a quick manual test and as far as I can tell this is working great.
Comment #12
chrisjlee CreditAttribution: chrisjlee commentedSure thing. i couldn't figure out how to get the interdiff to display that i git mv'd the file.
Oh well. this is me telling you that i did that.
Comment #13
oresh CreditAttribution: oresh commentedworks as intended. Waiting for patach to become green.
After it's added to core, we should use this markup and clean up css for it :
Comment #14
star-szr@oresh - thank you.
This looks great and I just did a manual test with JavaScript disabled to confirm. Good to go, thanks @chrisjlee!
Comment #15
LewisNyman CreditAttribution: LewisNyman commentedRELATED: #1989480: Progress Bar style update
Comment #16
alexpottPostponed on #1989480: Progress Bar style update
Comment #17
thedavidmeister CreditAttribution: thedavidmeister commentedComment #18
falcon03 CreditAttribution: falcon03 commented#12: 1939100-Convert_theme_progress_bar-12.patch queued for re-testing.
Comment #19
falcon03 CreditAttribution: falcon03 commentedThe blocker of this issue was committed, so sending patch for re-testing.
Comment #21
jenlamptonneeded a reroll
Comment #22
jenlamptonthat's not the right patch, trying again :)
Comment #23
oresh CreditAttribution: oresh commentedLooks great for me.
After closing this issue, please take a look at #1982220: Markup for: theme_progress_bar(), thanks :)
Comment #24
LewisNyman CreditAttribution: LewisNyman commentedIt looks like the twig file is using the old mark up, this would cause a visual regression now that the new progress bar style is in. It should be a simple task to add the correct classes, it would be good for someone looking to get more familiar with Twig.
Comment #25
oriol_e9gComment #26
oriol_e9g*mistaken patch*
Comment #27
oriol_e9gOps! O.o
Comment #28
oriol_e9gComment #29
star-szrNice! Tagging because this will need profiling before RTBC. For this one manually invoking from template_preprocess_page() or similar is probably the easiest way.
Comment #30
LewisNyman CreditAttribution: LewisNyman commentedThe patch looks great, applies fine. Do I set this to RTBC before profiling?
Comment #31
star-szr@LewisNyman - Not yet, but that's great to know that it's otherwise ready! Removing 'Novice' tag.
Comment #32
star-szrMinor thing that can be cleaned up:
We need to remove these lines now, see #2013094: [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file.
Comment #33
oriol_e9gComment #34
star-szrNice, thanks @oriol_e9g! Re-tagging for manual testing. I will do the testing and profiling in the next few days unless someone else beats me to it :)
Comment #36
star-szr#33: twig-progress-bar-1939100-33.patch queued for re-testing.
Comment #37
star-szrProfiling looks great, and I completed the manual testing, markup matches up. I'd say this one is ready to go!
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51fdbca2b5409&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51fdbca2b5409&...
Comment #38
alexpottCommitted 17c92fb and pushed to 8.x. Thanks!
Comment #39.0
(not verified) CreditAttribution: commentedUpdate remaining