Problem/Motivation

During development of the 8.x branch of Search API. We discovered another user for the progress bar component outside of the batch api:
#2252485: Improve UI for server/index summary pages

The progress bar is used to show a static progress, it does not update. So the animation does not make sense

Proposed resolution

Create a new variant class that removes the animation

Remaining tasks

Write a patch

User interface changes

None in core

API changes

A new HTML class

Files: 
CommentFileSizeAuthor
#51 2277551-51.drupal.progress-static.patch3.77 KBjoachim
#46 interdiff-2277551.txt1.66 KBsegi
#46 add_a_static_variant-2277551-46.patch3.77 KBsegi
#41 interdiff-39-41.txt532 bytesjp.stacey
#41 add_a_static_variant-2277551-41.patch4.16 KBjp.stacey
#39 add_a_static_variant-2277551-39.patch4.21 KBjp.stacey
#32 static_by_argument-2277551-32.patch3.56 KBx3cion
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,174 pass(es), 502 fail(s), and 63 exception(s). View
#32 static_by_argument--$overhaul-2277551-32.patch4.92 KBx3cion
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,165 pass(es), 502 fail(s), and 63 exception(s). View
#29 2277551.29.patch3.42 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2277551.29.patch. Unable to apply patch. See the log in the details link for more information. View
#29 25-29-interdiff.txt3.76 KBalexpott
#25 add_a_static_variant-2277551-25.patch1.99 KBleonrenkema
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,025 pass(es). View
#21 interdiff.txt763 bytesx3cion
#21 2277551-static_by_argument-21.patch1.97 KBx3cion
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2277551-static_by_argument-21.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

Dragan Eror’s picture

Issue tags: +Novice

Tagging...

x3cion’s picture

FileSize
1.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,693 pass(es). View

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.

x3cion’s picture

Status: Active » Needs review
x3cion’s picture

FileSize
1.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,719 pass(es). View

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.

Dragan Eror’s picture

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>

x3cion’s picture

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?

Dragan Eror’s picture

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>

LewisNyman’s picture

Status: Needs review » Needs work

Yep that makes sense. .progress__bar--static or even .progress--static .progress__bar

x3cion’s picture

Status: Needs work » Needs review
FileSize
2.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,719 pass(es). View

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

Dragan Eror’s picture

Status: Needs review » Reviewed & tested by the community

Looks prefect :)

Dragan Eror’s picture

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! :)

LewisNyman’s picture

Status: Reviewed & tested by the community » Needs work

Hey, just two points:

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

    We don't need to have both classes in the selector. We can just use the --static class.

  2. +++ b/core/modules/system/templates/progress-bar.html.twig
    @@ -9,6 +9,7 @@
    + * - static: If true, use no stripe animation.
    

    This reads a little bit inconsistent compared to the other comments. I would recommend something like "Disable the progress bar animation"

x3cion’s picture

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.

Dragan Eror’s picture

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

x3cion’s picture

FileSize
2.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,752 pass(es). View

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

x3cion’s picture

Status: Needs work » Needs review
x3cion’s picture

Assigned: Unassigned » x3cion
Dragan Eror’s picture

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.

Dragan Eror’s picture

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

tstoeckler’s picture

+++ 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?

x3cion’s picture

FileSize
1.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2277551-static_by_argument-21.patch. Unable to apply patch. See the log in the details link for more information. View
763 bytes

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.

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

I think this code is correct now. Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 2277551-static_by_argument-21.patch, failed testing.

leonrenkema’s picture

I've taken a look why the patch is failing and it looks like the theme.inc has changed in between and now the patch is out of date.

git  apply -v 2277551-static_by_argument-21.patch 
Checking patch core/includes/theme.inc...
error: while searching for:
      'variables' => array('url' => NULL, 'title' => NULL)
    ),
    'progress_bar' => array(
      'variables' => array('label' => NULL, 'percent' => NULL, 'message' => NULL),
      'template' => 'progress-bar',
    ),
    'indentation' => array(

error: patch failed: core/includes/theme.inc:2582
error: core/includes/theme.inc: patch does not apply

A line has been added on theme.inc:2289

'feed_icon' => array(
      'variables' => array('url' => NULL, 'title' => NULL),
      'template' => 'feed-icon',
    ),
    'progress_bar' => array(
      'variables' => array('label' => NULL, 'percent' => NULL, 'message' => NULL),
      'template' => 'progress-bar',
    ),
leonrenkema’s picture

Status: Needs work » Needs review
FileSize
1.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,025 pass(es). View

I've added an re-roll because the patch was failing.

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

Looks like the issues are resolved.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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.

LewisNyman’s picture

@alexpott I think technically I agree, but I think I always felt that it's important to display the animated progress bar as the 'default recommended progress bar'. Developers should only use the static progress bar in exceptional circumstances.

What do people think?

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.76 KB
3.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2277551.29.patch. Unable to apply patch. See the log in the details link for more information. View

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?

LewisNyman’s picture

Status: Needs review » Needs work
+++ b/core/misc/progress.js
@@ -25,6 +25,9 @@
+    if (drupalSettings.progressBarAnimate) {
+      $(this.element).find('div.progress__bar').addClass('progress__bar--animate');
+    }

Ok, this makes sense from an API point of view. I think we can tweak this Javascript to be a little more performant. I'll take a look

Cottser’s picture

Issue tags: +JavaScript
x3cion’s picture

FileSize
4.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,165 pass(es), 502 fail(s), and 63 exception(s). View
3.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,174 pass(es), 502 fail(s), and 63 exception(s). View

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().

x3cion’s picture

Status: Needs work » Needs review

The last submitted patch, 32: static_by_argument--$overhaul-2277551-32.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 32: static_by_argument-2277551-32.patch, failed testing.

x3cion queued 29: 2277551.29.patch for re-testing.

The last submitted patch, 29: 2277551.29.patch, failed testing.

jp.stacey’s picture

Assigned: x3cion » Unassigned
Issue tags: +SprintWeekend2016

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

jp.stacey’s picture

Status: Needs work » Needs review
FileSize
4.21 KB

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.

Status: Needs review » Needs work

The last submitted patch, 39: add_a_static_variant-2277551-39.patch, failed testing.

jp.stacey’s picture

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

jp.stacey’s picture

Status: Needs work » Needs review
droplet’s picture

  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');
    +    }
    
  3. no DIV scoped .progress__bar

  4. +++ 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

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

    standard rule should be the last one in ordering.

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

    no -moz i think

David Hernández’s picture

Status: Needs review » Needs work
segi’s picture

Assigned: Unassigned » segi
segi’s picture

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’s picture

Status: Needs work » Needs review
jp.stacey’s picture

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

emma.maria’s picture

Assigned: segi » Unassigned

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

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joachim’s picture

Component: Seven theme » theme system
Status: Needs review » Needs work
FileSize
3.77 KB

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.

Manjit.Singh’s picture

Status: Needs work » Needs review

triggering the test bot.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

Status: Needs review » Needs work

Back to needs work for #51

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.