Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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"> </div>' +
'<div class="progress__track"><div class="progress__bar"></div></div>' +
'<div class="progress__percentage"></div>' +
'<div class="progress__description"> </div>');
};
Proposed resolution
Use Drupal.theme instead of just hardcoding the HTML.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#18 | 2452381-18.patch | 479 bytes | Lendude |
#13 | use_drupal_theme_for-2452381-5.patch | 1.54 KB | lanchez |
#5 | use_drupal_theme_for-2452381-5.patch | 1.54 KB | PieterJanPut |
#7 | interdiff-2452382-5-7.txt | 933 bytes | lanchez |
#7 | use_drupal_theme_for-2452381-7.patch | 1.56 KB | lanchez |
Comments
Comment #1
lanchez CreditAttribution: lanchez commentedLooking this tomorrow.
Comment #2
googletorp CreditAttribution: googletorp commentedI've added a patch for this.
Comment #3
lanchez CreditAttribution: lanchez commentedComment #4
nod_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:
Just less
html += '';
, thanks!Comment #5
PieterJanPut CreditAttribution: PieterJanPut at iO commentedComment #6
googletorp CreditAttribution: googletorp commented#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.
Comment #7
lanchez CreditAttribution: lanchez commentedWell, what about array of strings and using join()? The performance difference can't be that high still it's imo more readable than before.
Comment #8
googletorp CreditAttribution: googletorp commented+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.
Comment #9
nod_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 :)
Comment #10
googletorp CreditAttribution: googletorp at Reveal IT commentedComment #11
googletorp CreditAttribution: googletorp at Reveal IT commentedComment #12
lanchez CreditAttribution: lanchez commentedI 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).
Comment #13
lanchez CreditAttribution: lanchez commentedAnd here's patch 5 again :)
Comment #14
googletorp CreditAttribution: googletorp at Reveal IT commentedOk, we all agree that this is RTCB now?
Comment #15
nod_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.
Comment #17
alexpottThis 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!
Comment #18
LendudeThe 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.
Comment #19
nod_Ow thanks for finding that.
Break ui so major at least, sorry about that.
Comment #20
tstoecklerThis also affects the installer, which looks very, very broken currently.
Comment #21
webchickJinkies. Thanks for catching that.
Committed and pushed to 8.0.x. Thanks!