Refactor push list HTML generation

RESOLVED FIXED

Status

RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

Details

Attachments

(3 attachments)

(Assignee)

Description

8 years ago
Created attachment 497892 [details] [diff] [review]
part 1, v1
Attachment #497892 - Flags: review?(arpad.borsos)
(Assignee)

Comment 1

8 years ago
Created attachment 497900 [details] [diff] [review]
part 2, v1

Make UserInterface able to regenerate an arbitrary push and to insert it at the correct position if it's new.
Attachment #497900 - Flags: review?(arpad.borsos)
Comment on attachment 497892 [details] [diff] [review]
part 1, v1

>+  _buildHTMLForPushPatches: function UserInterface__buildHTMLForPushPatches(push) {

Honestly, I don’t see a reason for this method. The patches of a push won’t ever change.

>-    var nodeHtml = '<li id="push-' + push.patches[0].rev + '">\n' +
>+    var nodeHtml = '<li class="push" id="push-' + push.id + '" data-id="' + push.id + '">\n' +


Our push objects don’t have an id member (yet). Is this mixed up with some other changes in your queue?
Instead of push.patches[0].rev you can use push.toprev

Also, the class and data-id don’t seem to be in use yet.


>+  _refreshPushResultsInPushNode: function UserInterface__refreshPushResultsInPushNode(push, node) {
>+    $(".results", node).html(this._buildHTMLForPushResults(push));
>+  },

You could make use of this method instead of the

        if (exists)
          $(exists).replaceWith(self._generatePushNode(push));

blocks in the update and history load codepaths, that way we can avoid recreating some of the DOM.
Attachment #497892 - Flags: review?(arpad.borsos) → review-
(Assignee)

Comment 3

8 years ago
Created attachment 497905 [details] [diff] [review]
part 3, v1

Don't pass machines to the success callback. Get them via a public method on Data when needed.
Attachment #497905 - Flags: review?(arpad.borsos)
Comment on attachment 497900 [details] [diff] [review]
part 2, v1

>+      var currentPushNodeID = currentPushNode.attr("data-id");
>+      if (currentPushNodeID === undefined)
>+        break;

Why do we need the data attribute at all? Isn’t the id enough? And why the undefined check? .push children of #pushes are guaranteed to have id == data-id, don’t they?

>+  _getPushNode: function UserInterface__getPushNode(push) {
>+    return $("#push-" + push.id);
>+  },

I’m not a fan of adding an indirection for such a trivial function.
Attachment #497900 - Flags: review?(arpad.borsos) → review+
Comment on attachment 497892 [details] [diff] [review]
part 1, v1

It all makes sense now :)
Attachment #497892 - Flags: review- → review+
(Assignee)

Comment 6

8 years ago
(In reply to comment #2)
> Comment on attachment 497892 [details] [diff] [review]
> part 1, v1
> 
> >+  _buildHTMLForPushPatches: function UserInterface__buildHTMLForPushPatches(push) {
> 
> Honestly, I don’t see a reason for this method. The patches of a push won’t
> ever change.

Factoring it out makes _generatePushNode shorter and easier to understand, don't you think?

> Instead of push.patches[0].rev you can use push.toprev

Good catch!
(Assignee)

Comment 7

8 years ago
(In reply to comment #4)
> Comment on attachment 497900 [details] [diff] [review]
> part 2, v1
> 
> >+      var currentPushNodeID = currentPushNode.attr("data-id");
> >+      if (currentPushNodeID === undefined)
> >+        break;
> 
> Why do we need the data attribute at all? Isn’t the id enough?

Which id are you referring to?

> And why the
> undefined check? .push children of #pushes are guaranteed to have id ==
> data-id, don’t they?

Good point, I wonder why I added that.

> >+  _getPushNode: function UserInterface__getPushNode(push) {
> >+    return $("#push-" + push.id);
> >+  },
> 
> I’m not a fan of adding an indirection for such a trivial function.

Agreed. I thought I was going to use it more often, but I'm not.
Well ok, the difference is that id has a "push-" prefix and data-id has not. But a string.replace() should easily take care of that.
Attachment #497905 - Flags: review?(arpad.borsos) → review+
(Assignee)

Comment 10

8 years ago
Actually, the other parts that are coming don't belong to this bug because they don't have much to do with HTML generation. This is fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Blocks: 619651
Product: Webtools → Tree Management
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.