Add a 'static' variant to the progress bar

Tagging...

Hello everyone,

I'm pretty new to this and I've to admit that I read or found no style guide or anything else.

I made up this patch and a patch for #2252485: Improve UI for server/index summary pages to implement it.

I discovered that the animation is not set for anything but -webkit and -moz, so I think it would be a good idea to do that...? However, I removed them for the static progress bar, just in case.

Of course, I have a patch for that already, too and the problem about the animation going in the wrong direction is no big deal either.
Though if I got it right, changing the animation direction is not part of this issue.

If anything's wrong, please specify what and how to do it better, I'm eager to learn... or so.

I'm sorry for the double post, but I've just read https://drupal.org/coding-standards#indenting and noticed that I used tabs in the CSS, here's the updated patch.

Thanks @x3cion, looks great!

Few more notes:

1. +++ b/core/modules/system/css/system.theme.css
   @@ -357,6 +357,14 @@ th.checkbox {
   +.progress__bar.static {
   

   We don't want to introduce NEW "static" class into core.

   Use '.progress__bar--static' selector instead '.progress__bar.static'.

   You can see formatting guideline here https://drupal.org/node/1887918#formatting

2. +++ b/core/modules/system/templates/progress-bar.html.twig
   @@ -17,7 +17,7 @@
   +  <div class="progress__track"><div class="progress__bar {% if static %}static{% endif %}" style="width: {{ percent }}%"></div></div>
   

   Also change this into

   <div class="progress__track"><div class="{% static ? 'progress__bar--static' : 'progress__bar' %}" style="width: {{ percent }}%"></div></div>

I got a question to that.

https://drupal.org/node/1887918#formatting says

<!--
  Modifier classes create component variants; modifier classes must
  be used with the original class in the markup
  -->

Does that mean I would use

<div class="progress__track"><div class="progress__bar {{ static ? 'progress__bar--static' : '' }}" style="width: {{ percent }}%"></div></div>

or something similiar? If i don't use it like this, wouldn't this create duplicate CSS, which probably is not intended?

Seams you're right, I missed that...
So, then would be just:
<div class="progress__track"><div class="progress__bar {% if static %}progress__bar--static{% endif %}" style="width: {{ percent }}%"></div></div>

This patch should take everything in account. I also added the static argument to the documentation comment.

Looks prefect :)

Just one tip for the future: theres is convention for patch naming [project_name]-[short-description]-[issue-number]-[comment-number].patch
The acceptable one is also [issue-number]-[short-description]-[comment-number].patch

And file shouldn't start with hash character (it is strange that testing works at all). Should start with small alphabet character or number.

Cheers! :)

About 1.: Isn't it insecure to rely on the weak precedence of class after class css declaration? .progress__bar--static is a .progress__bar too. If someone changes the order in the css file, it might break the css if we only use .progress__bar--static.

@x3cion no worries, just place your code after .progress__bar too, no one will change order. If so, there will be BUG and issue, and will be fixed again. ;)

About the convention for patch naming: By "comment-number" you mean the supposed comment number of the post that this patch will be with?

About the convention for patch naming: By "comment-number" you mean the supposed comment number of the post that this patch will be with?

Yes. So, even if same patch (the content of file) is uploaded twice, the file name should be updated.

For example, the file name of patch here in this comment should be something like: 2277551-example-patch-name-19.patch

+++ b/core/modules/system/templates/progress-bar.html.twig
@@ -17,7 +18,7 @@
+  <div class="progress__track"><div class="progress__bar {% if static %}progress__bar--static{% endif %}" style="width: {{ percent }}%"></div></div>

I'm no Twig expert, but doesn't this leave an empty space in the markup if static is FALSE?

Hi,

you're right and it might be a cleaner solution to insert the space only if neccessary, so I changed my patch once more and moved the space inside the condition.

Don't be surprised, but for the sake of testing I'll attach an interdiff too.

Looks like the issues are resolved.

This approach feels the wrong way around - like a double negative. How about having an animate variable that defaults to TRUE and then have a class that adds something rather than removes something.

Well the current change proposed in #25 doesn't actually work with the ajax form of the progress bar anyway since the html is generated in progress.js.

How about something like this?

I took the Patch from #29, but it didn't apply properly for theme.inc and batch.inc. I searched and searched why it wouldn't apply properly, but didn't find anything on my side. I patched an up to date working copy and did the rest of the changes manually.

Two little questions:

1. Why don't you use jQuery object naming convetions like this.$element for example? This would make clear if it's an jQuery object or not. Anyways, from what I see, this.element is initiated as a jQuery object and will always be. Is there a reason it's re-jquerified all the time?
2. Is there a reason to do div.progress__bar instead of only .progress__bar in the selector? From my understanding of Drupal's Classing Conventions (heh.), it should be clear it can only be this very div.

Ok, so I got two patches here. The thing is, I did the renaming of element to $element and changed the usage of it in all of progress.js, mainly using .find() instead of $(selector, context). I did not change the div.class selectors, as I don't know if I'm already going too far with this one - especially for a ticket that's about a different issue. Since it's already off-topic, I put it in a second patch.

To the real problem that needed work: I integrated the check for drupalSettings.progressBarAnimate into the .html() call. The always-adding of an extra string (: '') should be tolerable, since it circumvents a costly .find().

Unassigning from previous owner, as it's been left for a year or so; looking at this for Sprint Weekend.

A lot has changed since the patch in #29, so much that it's not really possible to create an interdiff (as it won't apply HEAD any more.) But please find attached an updated patch, that just takes #29 and makes the following changes:

• core/includes/theme.inc: standardizes the Implements comment.
• core/misc/progress.js: copes with the fact the existing code now uses Drupal.theme('progressBar').
• core/modules/system/templates/progress-bar.html.twig: the CSS for this is now in classy at core/themes/classy/css/components/progress.css; also made the [dir=rtl] animation CSS dependent on the new --animate class.

Among other things, adding of Javascript settings has changed. Re-rolling patch to hopefully deal with that bit (not clear where all the errors are coming from.)

1. +++ b/core/includes/theme.inc
   @@ -1837,3 +1837,14 @@ function drupal_common_theme() {
   +  $variables['#attached']['drupalSettings']['progressBarAnimate'] = $variables['animate'];
   
   +++ b/core/misc/progress.js
   @@ -53,6 +53,11 @@
   +    if (drupalSettings.progressBarAnimate) {
   +      $(this.element).find('div.progress__bar').addClass('progress__bar--animate');
   +    }
   
   +++ b/core/modules/system/templates/progress-bar.html.twig
   @@ -17,7 +21,7 @@
   +  <div class="progress__track"><div class="progress__bar{% if animate %} progress__bar--animate{% endif %}" style="width: {{ percent }}%"></div></div>
   

   Anyone can explain these lines ? Why both in front & backend.

2. +++ b/core/misc/progress.js
   @@ -53,6 +53,11 @@
   +    if (drupalSettings.progressBarAnimate) {
   +      $(this.element).find('div.progress__bar').addClass('progress__bar--animate');
   +    }
   

no DIV scoped .progress__bar

3. +++ b/core/themes/classy/css/components/progress.css
   @@ -44,22 +44,26 @@
   +  -webkit-animation: animate-stripes 3s linear infinite;
   +  -moz-animation: animate-stripes 3s linear infinite;
   

   missing standard one

4. +++ b/core/themes/classy/css/components/progress.css
   @@ -44,22 +44,26 @@
   +  animation-direction: reverse;
   

   standard rule should be the last one in ordering.

5. +++ b/core/themes/classy/css/components/progress.css
   @@ -44,22 +44,26 @@
   +  -moz-animation-direction: reverse;
   

   no -moz i think

I think @jp.stacey add the progress__bar--animate class on the backend and the frontend side as well, because he prepared for no-js case.

@segi @droplet I was only re-rolling the patch, so I can't comment on why that was there! But, yes, as @segi says there are already two instances of the progress bar markup, for when Javascript needs to create a progress bar for itself. So that does make sense.

Setting this issue to unassigned to open up @segi's patch for review :)

This works great. Spoke too soon! The 'available updates' progress bar (at admin/reports/updates) no longer animates with this patch!

The attached is a plain reroll of #46.

However... it seems a bit odd to me to have the striping show when the bar is un-animated. It seems to me that their whole purpose is to be part of the animated effect.

Search API's version of a static progress bar removes the background too, with its own CSS.

triggering the test bot.

Back to needs work for #51

Only patch reroll

Only re-rolled latest patch onto 9.1 branch

I have updated the patch as earlier one failed.

The documentation in the base template in core/modules/system/templates/progress-bar.html.twig needs to be updated too:

/**
 * @file
 * Theme override for a progress bar.
 *
 * Note that the core Batch API uses this only for non-JavaScript batch jobs.
 *
 * Available variables:
 * - label: The label of the working task.
 * - percent: The percentage of the progress.
 * - message: A string containing information to be displayed.
 */

Test case failure fixed.

In #71

The documentation in the base template in core/modules/system/templates/progress-bar.html.twig needs to be updated too:

Already added in #64

  * - label: The label of the working task.
  * - percent: The percentage of the progress.
  * - message: A string containing information to be displayed.
+ * - animate: If set, enable the progress bar animation.
+ *
+ * @see template_preprocess_progress_bar()
+ * @see progress.js
  *
  * @ingroup themeable
  */

Oops, I must have missed that in the patch!

Tests pass, LGTM.

+++ b/core/misc/progress.js
@@ -16,6 +16,11 @@
+
+    // Implement optional animation of progress bar.
+    if (drupalSettings.progressBarAnimate) {
+      $(this.element).find('progress__bar').addClass('progress__bar--animate');
+    }

We need to change progress.es6.js and then use the js build tools to update this file. See https://www.drupal.org/node/2815083

yarn run v1.19.1
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/alex/dev/sites/drupal8alt.dev/core/misc/progress.es6.js
[10:14:32] '/Users/alex/dev/sites/drupal8alt.dev/core/misc/progress.es6.js' is being checked.
[10:14:33] '/Users/alex/dev/sites/drupal8alt.dev/core/misc/progress.es6.js' is not updated.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

profiles/demo_umami/themes/umami/css/classy/components/progress.css
 53:3  ✖  Expected "-webkit-transition" to come before "animation"   order/properties-order
 60:1  ✖  Expected empty line before at-rule                         at-rule-empty-line-before

PHPCS: core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php passed

themes/bartik/css/classy/components/progress.css
 53:3  ✖  Expected "-webkit-transition" to come before "animation"   order/properties-order
 60:1  ✖  Expected empty line before at-rule                         at-rule-empty-line-before


themes/classy/css/components/progress.css
 53:3  ✖  Expected "-webkit-transition" to come before "animation"   order/properties-order
 60:1  ✖  Expected empty line before at-rule                         at-rule-empty-line-before


themes/seven/css/classy/components/progress.css
 53:3  ✖  Expected "-webkit-transition" to come before "animation"   order/properties-order
 60:1  ✖  Expected empty line before at-rule                         at-rule-empty-line-before

The css changes are also not compatible with our rules.

Thanks @alexpott for review. Adding suggestion from #74.

bad test bot :D

Adding correct hash for file.

Since patch applied cleanly, I just queued a retest of #77 for 9.2.x.

:)

try to fix the custom commands failure

Updated

Fixing the checksum error. Hopefully should be green.

// Implement optional animation of progress bar.
On this case we should use
// Implements optional animation of progress bar.

Made the change suggested in #86. Please review.

Hello, just tried patch #87. Didn't found any more grammar errors, and the patch seems to be working ok, so I'm moving the issue to RTBC

I fully agree. +1 to it

Please do not ask the testbot to try again until #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed.

#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest was committed. Ordered retest and put this issue back to RTBC per #88.

1. +++ b/core/includes/theme.inc
   @@ -2135,3 +2135,14 @@ function drupal_common_theme() {
   +  $variables['#attached']['drupalSettings']['progressBarAnimate'] = $variables['animate'];
   

   How about scenarios where multiple progress bars exist on same page?

2. +++ b/core/misc/progress.es6.js
   @@ -57,6 +57,11 @@
   +      $(this.element).find('progress__bar').addClass('progress__bar--animate');
   

   Is there a way this could be added to Drupal.theme.progressBar since the markup should be overridable.

3. +++ b/core/modules/system/templates/progress-bar.html.twig
   @@ -17,7 +21,7 @@
   +  <div class="progress__track"><div class="progress__bar{% if animate %} progress__bar--animate{% endif %}" style="width: {{ percent }}%"></div></div>
   

   This changes the default progress bar to be without animation which doesn't match with the issue summary which says it should be a new variant. This would lead into change of behavior for any template overrides.

@lauriii nice catch on the multiple bars on the same page. That scenario feels worth adding a test for.