As part of #1415788: Javascript winter clean-up we're cleaning up the jQuery selectors (remove duplicated code, simplify selectors).

Removed jQuery wrapper from an element that already is a jQuery object.
Pruposely didn't change this.element to this.$element as per #1460022: Prefix all jQuery variables with $, and because it might break other js files as it is a property of Drupal.ProgressBar

Files: 

Comments

sxnc’s picture

Downloaded and ran this patch on my local, no errors and everything still seems to work as before. Patch looks good too as far as my knowledge goes~

nod_’s picture

Status: Needs review » Needs work

you can rename the .element property as well, the other uses are in ajax.js (which needs work longer-term) and batch.js which is already cleaned up.

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
2.46 KB
PASSED: [[SimpleTest]]: [MySQL] 40,707 pass(es). View

Patch based on #2

nod_’s picture

Status: Needs review » Needs work

missing the change to ajax.js and batch.js seems to be working but should be changed anyway :)

Otherwise, all good, thanks!

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
5.04 KB
PASSED: [[SimpleTest]]: [MySQL] 40,703 pass(es). View

Woops, you're right. Another try ;-)

nod_’s picture

droplet’s picture

Drupal.ProgressBar = function (id, updateCallback, method, errorCallback) {
  var pb = this; // unused.
+++ b/core/misc/progress.jsundefined
@@ -21,8 +21,8 @@ Drupal.ProgressBar = function (id, updateCallback, method, errorCallback) {
+  this.$element = $('<div class="progress" aria-live="polite"></div>').attr('id', id);
+  this.$element.html('<div class="bar"><div class="filled"></div></div>' +

chaining ?

$().attr().html()
hansyg’s picture

Assigned: Unassigned » hansyg

I can reroll this with the chain

hansyg’s picture

Assigned: hansyg » Unassigned

Patch seems to be failing, can't apply. Un assigning myself needs someone else to review

hansyg’s picture

FileSize
4.38 KB
PASSED: [[SimpleTest]]: [MySQL] 55,710 pass(es). View

I lied I re-rolled the patch above.

Status: Needs review » Needs work
Issue tags: -JavaScript clean-up, -Needs JS testing

The last submitted patch, core-js-1751044-9.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review

#10: core-js-1751044-9.patch queued for re-testing.

areke’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

This patch needs another re-roll.

martin107’s picture

Status: Needs work » Needs review
FileSize
3.83 KB
PASSED: [[SimpleTest]]: [MySQL] 59,676 pass(es). View

reroll

droplet’s picture

FileSize
5.21 KB
PASSED: [[SimpleTest]]: [MySQL] 59,889 pass(es). View

- Reroll patch
- caches selectors
- simplify errorMarkup

nod_’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll

umm so two things about:

  this.$element.progress__bar = this.$element.find('div.progress__bar');
  this.$element.progress__percentage = this.$element.find('div.progress__percentage');
  this.$element.progress__description = this.$element.find('div.progress__description');
  this.$element.progress__label = this.$element.find('div.progress__label');

That's adding jQuery elements to a jQuery element, not too happy about that. It should be on the progressBar object directly:

  this.$progress__bar = this.$element.find('div.progress__bar');
  // maybe?
  this.$progressBar = this.find();

  // or even but I'd prefer the version above.
  this.elements = {
    'progress__bar': XXX
  };

Then for the elements I think we could create them individually and build the structure with .append(), it's a detached DOM node so there won't be a huge perf impact.

  this.$progressBar = $('<div class="">');

  // and later
  this.$element.append(this.$progressBar).append(…);
droplet’s picture

Status: Needs work » Needs review
FileSize
3.95 KB
5.76 KB
PASSED: [[SimpleTest]]: [MySQL] 59,893 pass(es). View

Ouch!! Previous patches do not work. Added some fixes & your suggestions. Thanks.

droplet’s picture

FileSize
5.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1751044-progressJS.patch. Unable to apply patch. See the log in the details link for more information. View

Reroll, fixed space indentation

nod_ queued 18: 1751044-progressJS.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 18: 1751044-progressJS.patch, failed testing.

nod_’s picture

Issue tags: +Needs reroll
Manjit.Singh’s picture

FileSize
5.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,782 pass(es). View

rerolling a patch :)

Manjit.Singh’s picture

Status: Needs work » Needs review
googletorp’s picture

Issue tags: -Needs reroll
droplet’s picture

Priority: Normal » Minor
Status: Needs review » Needs work

Thanks for great work! I reviewed it again. Most issues already addressed in other issues. Just few `this.element to this.$element` still left-over there.

mandar.harkare’s picture

Assigned: Unassigned » mandar.harkare
Issue tags: +drupalconasia2016
mandar.harkare’s picture

Please review the patch.

mandar.harkare’s picture

Status: Needs work » Needs review
googletorp’s picture

Status: Needs review » Needs work

There's a lot of examples where code is not fully optimized and looks like it's broken in a few a few places as well.

The idea is to save $element once so we don't have to create new jQuery objects all of the time. There are some places where code is still not doing that and some of the stuff doesn't really make sense.

  1. +++ b/core/misc/ajax.js
    @@ -312,25 +312,25 @@
    +    if (this.$element && this.element.form) {
    

    Should be

    this.$element && this.$element.form

  2. +++ b/core/misc/ajax.js
    @@ -312,25 +312,25 @@
    +      else if (this.$element && $element.form) {
    

    Why test for this.$element here, should be $element

  3. +++ b/core/misc/ajax.js
    @@ -688,35 +688,35 @@
    +    var progressBar = new Drupal.ProgressBar('ajax-progress-' + $(this.element).id, $.noop, this.progress.method, $.noop);
    

    Don't use $(this.element) use this.$element instead

  4. +++ b/core/misc/progress.js
    @@ -66,11 +66,11 @@
    +      $('div.progress__description', this.$element).html(message);
    +      $('div.progress__label', this.$element).html(label);
    

    This should use the jQuery element,

    this.$element.find().html();

nod_’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review
Issue tags: +JavaScript clean-up
FileSize
6.18 KB

Lots of confusion here. The purpose of this issue is to change the progress bar object, not the whole ajax framework. I've rerolled a patch with only the changes in scope.

This is 8.1 material.

googletorp’s picture

Status: Needs review » Needs work
+++ b/core/misc/progress.js
@@ -8,32 +8,15 @@
-   * Theme function for the progress bar.
-   *
-   * @param {string} id
-   *
-   * @return {string}
-   *   The HTML for the progress bar.
-   */
-  Drupal.theme.progressBar = function (id) {
-    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>';
-  };
-
-  /**

Why are we removing the use of a theme function. Don't we want themes to be able to customize the look and feel of the progressbar?

It looks like the patch just changes $this.element to $this.$element, is this a general coding style or why are we doing this? I can't see the actual selectors are changed which is what I read is part of the goal for this patch

lazyD’s picture

Please check the attached patch file.

Thanks.

lazyD’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 32: core-js-progress-cleanup-1751044-32.patch, failed testing.

lazyD’s picture

Took checkout against and then generated the patch.
Please check this.

lazyD’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 35: core-js-progress-cleanup-1751044-34.patch, failed testing.

lazyD’s picture

First Commit, Please consider my apologies for repeated attempt to review.

Followed the steps on https://www.drupal.org/node/707484

Status: Needs review » Needs work

The last submitted patch, 38: drupal_core-core-js-progress-cleanup-1751044-34-8.patch, failed testing.

lazyD’s picture

Need Help.

Can someone guide me why bot is rejecting my patch?

Thanks

martin107’s picture

Hey lazyD

the patch needed rerolling [ https://www.drupal.org/patch/reroll ]

I took the patch from #38 and followed the reroll procedures going back to the date form the "last known good patch" - February 21, 2016 at 1:22pm
the reroll was automatic with no need for manual tweaking.

martin107’s picture

Status: Needs work » Needs review
lazyD’s picture

thanks Martin107.

How can i understand whether it requires a rerolling? Sorry but am bad with git.

martin107’s picture

The patch is series of code changes.
The underlying code in core is updated - invalidating the change instructions.

So "git apply" - fails. The solution is to go back to a set of code where the patch works - that is the revert bit. next is the rebase

When rebasing hits a conflict ... there are conflicting changes to a function/method .. the automated merging stops - dumps both version of the code into the place of conflict and forces the users to sort things out.

I hope that makes understanding the article in #41 easier.

lazyD’s picture

Assigned: mandar.harkare » Unassigned

marking as unassigned, so someone can test and patch can be committed.

Manjit.Singh’s picture

@lazyD , @martin107 and patch is still giving an issue, it is giving me a whitespace issue.

terminal

Manjit.Singh’s picture

resolving the whitespace issue.

lazyD’s picture

@Manjit Thanks

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now 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.

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.