Problem/Motivation

progress.js outputs quite some JS, let's ensure that its themable, see

    this.element = $('<div class="progress" aria-live="polite"></div>').attr('id', id);
    this.element.html('<div class="progress__label">&nbsp;</div>' +
      '<div class="progress__track"><div class="progress__bar"></div></div>' +
      '<div class="progress__percentage"></div>' +
      '<div class="progress__description">&nbsp;</div>');
  };

Proposed resolution

Use Drupal.theme instead of just hardcoding the HTML.

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lanchez’s picture

Assigned: Unassigned » lanchez

Looking this tomorrow.

googletorp’s picture

Status: Active » Needs review
FileSize
1.58 KB

I've added a patch for this.

lanchez’s picture

Assigned: lanchez » Unassigned
nod_’s picture

Status: Needs review » Needs work

Missing a semicolon (eslint error) and it's also missing a return in the function.

All the concatenation is painful to look at, can you make it smaller? something like:


  return '<div id="' + id + '" class="progress" aria-live="polite">' +
    '<div class="progress__label">&nbsp;</div><div class="progress__track"><div class="progress__bar"></div></div>' +
    '<div class="progress__percentage"></div><div class="progress__description">&nbsp;</div>' +
    '</div>';

Just less html += '';, thanks!

PieterJanPut’s picture

Status: Needs work » Needs review
Issue tags: +drupaldevdays
FileSize
1.54 KB
googletorp’s picture

#4 I like to have all of the html alligned, but either way is fine by me.

#5 Remember to post an interdiff.text so we can see what changes was made.

lanchez’s picture

Well, what about array of strings and using join()? The performance difference can't be that high still it's imo more readable than before.

googletorp’s picture

+1 on array method. I think this is a lot better and I've seen this method in other JS projects.

Since I'm partly author of the patch I won't change status, but imo this is RTCB.

nod_’s picture

Status: Needs review » Needs work

Personally I like the join stuff as well, but it's not appropriate here.

We don't use the pattern anywhere else in core and I don't want to encourage it, it's a clever workaround (clever being a bad thing here). If concatenation is an issue we can put everything in one line. It's ugly but it's not really a problem, it's easy enough to understand, there is nothing crazy going on, it's just the ID that is dynamic.

Can you reup #5 to have the last patch being the one that will be RTBC? thanks :)

googletorp’s picture

googletorp’s picture

lanchez’s picture

I think that dynamic id is not "the thing" here, but cleaner js templates through, for example, Drupal.theme. I agree that the array method is not probably appropriate here, but I think that there should be some kind of clean format for stuff like that in the core (without implementing more libraries).

lanchez’s picture

Status: Needs work » Needs review
FileSize
1.54 KB

And here's patch 5 again :)

googletorp’s picture

Status: Needs review » Reviewed & tested by the community

Ok, we all agree that this is RTCB now?

nod_’s picture

Yup :) thanks!

I'd rather not make a policy for that, in the end we really don't want HTML in our JS if we can avoid it.

  • alexpott committed 0abd1bb on 8.0.x
    Issue #2452381 by lanchez, googletorp, PieterJanPut: Use Drupal.theme...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 0abd1bb and pushed to 8.0.x. Thanks!

Lendude’s picture

Status: Fixed » Needs review
FileSize
479 bytes
+++ b/core/misc/progress.js
@@ -20,11 +35,7 @@
+    this.element = $(Drupal.theme(id));

The Drupal.theme call is missing the bit that says it should theme it as a progressBar, so the bar never shows up.
Not sure what the procedure is for this, open a new issue or tag it onto this one, but I'll start by adding it here.

nod_’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

Ow thanks for finding that.

Break ui so major at least, sorry about that.

tstoeckler’s picture

Priority: Major » Critical

This also affects the installer, which looks very, very broken currently.

webchick’s picture

Priority: Critical » Normal
Status: Reviewed & tested by the community » Fixed

Jinkies. Thanks for catching that.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 70890d6 on 8.0.x
    Issue #2452381 follow-up by Lendude: Fixed progress bar not showing up
    

Status: Fixed » Closed (fixed)

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