Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chrisjlee’s picture

Initial patch. Don't really know what i'm doing but i'd thought i'd give it a good try.

chrisjlee’s picture

Status: Active » Needs review

Change status.

star-szr’s picture

Status: Needs review » Needs work

@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.

star-szr’s picture

Issue tags: +Needs manual testing

And tagging, this will need manual testing.

nikkubhai’s picture

The 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. */

star-szr’s picture

This 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:

+++ b/core/includes/theme.incundefined
@@ -3198,6 +3178,7 @@ function drupal_common_theme() {
+      'template' => 'progress_bar',

This should be progress-bar instead of progress_bar.

chrisjlee’s picture

Assigned: Unassigned » chrisjlee
chrisjlee’s picture

Status: Needs work » Needs review
FileSize
2.07 KB
679 bytes

Status: Needs review » Needs work
Issue tags: -Needs manual testing, -Twig

The last submitted patch, 1939100-Convert_theme_progress_bar-8.patch, failed testing.

chrisjlee’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing, +Twig
star-szr’s picture

Thanks @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.

chrisjlee’s picture

Sure 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.

oresh’s picture

works 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 :

<div id="progress" class="progress">
<div class="progress-bar"><div class="progress-filled" style="width: {{ percent }}%;"></div></div>
<p class="progress-percentage">{{ percent }}%</p>
<p class="progress-message">{{ message }}</p>
</div>
star-szr’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

@oresh - thank you.

This looks great and I just did a manual test with JavaScript disabled to confirm. Good to go, thanks @chrisjlee!

LewisNyman’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Postponed
thedavidmeister’s picture

Assigned: chrisjlee » Unassigned
falcon03’s picture

Status: Postponed » Needs review
falcon03’s picture

The blocker of this issue was committed, so sending patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1939100-Convert_theme_progress_bar-12.patch, failed testing.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
8.88 KB

needed a reroll

jenlampton’s picture

that's not the right patch, trying again :)

oresh’s picture

Looks great for me.
After closing this issue, please take a look at #1982220: Markup for: theme_progress_bar(), thanks :)

LewisNyman’s picture

Status: Needs review » Needs work
Issue tags: +Novice, +html-novice
+++ b/core/modules/system/templates/progress-bar.html.twigundefined
@@ -0,0 +1,21 @@
+<div id="progress" class="progress">
+  <div class="bar"><div class="filled" style="width: {{ percent }}%"></div></div>
+  <div class="percentage">{{ percent }}%</div>
+  <div class="message">{{ message }}</div>

It 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.

oriol_e9g’s picture

Assigned: Unassigned » oriol_e9g
oriol_e9g’s picture

Assigned: oriol_e9g » Unassigned
FileSize
2.54 KB

*mistaken patch*

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
2.54 KB

Ops! O.o

oriol_e9g’s picture

star-szr’s picture

Issue tags: +needs profiling

Nice! Tagging because this will need profiling before RTBC. For this one manually invoking from template_preprocess_page() or similar is probably the easiest way.

LewisNyman’s picture

The patch looks great, applies fine. Do I set this to RTBC before profiling?

star-szr’s picture

Issue tags: -Novice

@LewisNyman - Not yet, but that's great to know that it's otherwise ready! Removing 'Novice' tag.

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Novice

Minor thing that can be cleaned up:

+++ b/core/modules/system/templates/progress-bar.html.twig
@@ -0,0 +1,25 @@
+ *
+ * @see template_preprocess()

We need to remove these lines now, see #2013094: [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
2.51 KB
star-szr’s picture

Issue tags: +Needs manual testing

Nice, 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 :)

Status: Needs review » Needs work
Issue tags: -Novice, -Needs manual testing, -needs profiling, -Twig, -html-novice

The last submitted patch, twig-progress-bar-1939100-33.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Needs manual testing, +needs profiling, +Twig, +html-novice

#33: twig-progress-bar-1939100-33.patch queued for re-testing.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice, -Needs manual testing, -needs profiling

Profiling looks great, and I completed the manual testing, markup matches up. I'd say this one is ready to go!

=== core-1939100-33..core-1939100-33 compared (51fdbca2b5409..51fdbd76c2eed):

ct  : 101,275|101,275|0|0.0%
wt  : 496,748|496,084|-664|-0.1%
cpu : 459,944|458,431|-1,513|-0.3%
mu  : 27,104,400|27,104,400|0|0.0%
pmu : 27,172,824|27,172,824|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51fdbca2b5409&...

=== core-1939100-33..progress-bar-1939100-33 compared (51fdbca2b5409..51fdbdbb58abc):

ct  : 101,275|101,357|82|0.1%
wt  : 496,748|497,696|948|0.2%
cpu : 459,944|459,872|-72|-0.0%
mu  : 27,104,400|27,134,536|30,136|0.1%
pmu : 27,172,824|27,203,528|30,704|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51fdbca2b5409&...

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 17c92fb and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Update remaining