Summary

Prevent duplicate infobox fetches

Review Request #8568 — Created Dec. 12, 2016 and submitted Dec. 16, 2016, 2:13 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

009ec7b...

Reviewers

Groups

reviewboard

People

Description

Previously, datagrids would attach additional event handler for
infoboxes on top of the initial event handler. This would result in two
fetches for all infoboxes in the datagrid until the first refresh. Now
we set an attribute on elements which can have infoboxes attached them
when the infobox handler has been added so that it will not be added
again.

Testing Done

Verified infoboxes in datagrids are no longer requested twice.

Issues

Description	From	Last Updated
Can we make this data-has-${id}-infobox? As-is it'll make attributes like data-has-user which isn't immediately obvious.	david	Dec. 12, 2016, 6:21 p.m.
Early returns increase complexity. Given that the code that would be skipped by it is only three statements, how about …	david	Dec. 12, 2016, 6:27 p.m.
This needs to be $target now, because fat-arrow functions have lexical this binding.	david	Dec. 12, 2016, 6:27 p.m.
We should use .data() instead for storing this, rather than using an attribute. We actually can attach the view as …	chipx86	Dec. 15, 2016, 12:05 p.m.

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/common.es6.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/common.es6.js

reviewboard/static/rb/js/common.es6.js (Diff revision 1)

The issue has been dropped. Show all issues

Can we make this data-has-${id}-infobox? As-is it'll make attributes like data-has-user which isn't immediately obvious.

brennie Dec. 12, 2016, 6:21 p.m.

No it will make data-has-user_infobox etc.

reviewboard/static/rb/js/common.es6.js (Diff revision 1)

The issue has been resolved. Show all issues

Early returns increase complexity. Given that the code that would be skipped by it is only three statements, how about we instead do:

if (!$target.attr(infoboxAttr)) {
    const view = new RB.InfoboxView({
        $target: $target,
        el: $el,
    });

    view.render();
    $target.attr(infoboxAttr, true);
}

reviewboard/static/rb/js/common.es6.js (Diff revision 1)

The issue has been resolved. Show all issues

This needs to be $target now, because fat-arrow functions have lexical this binding.

brennie Dec. 12, 2016, 6:21 p.m.
```
Good catch; totally meant to do this.
```

Change Summary:

Addressed David's issues.

Commit:

e3009a61e445df5878abb98178097b63d83a8131

009ec7bd346a1c4c27c5709d3ff38d187624e59d

Diff:

Revision 2 (+13 -6)

Show changes

reviewboard/static/rb/js/common.es6.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/common.es6.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/common.es6.js

Ship it!

```
Ship It!
```

reviewboard/static/rb/js/common.es6.js (Diff revision 2)

The issue has been resolved. Show all issues

We should use .data() instead for storing this, rather than using an attribute.
We actually can attach the view as the data. This is what jQuery widgets do. This allows for fetching the view for a given element as well.
(.data() doesn't simply wrap .attr('data-...'))

Change Summary:

Addressed Christian's issue

Diff:

Revision 3 (+13 -7)

Show changes

reviewboard/static/rb/js/common.es6.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/common.es6.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/common.es6.js

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-3.0.x (d2fdef8)