Summary

Refactor out the code for centering diff collapse buttons

Review Request #8444 — Created Sept. 27, 2016 and submitted Oct. 6, 2016, 1:07 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

d1df202...

Reviewers

Groups

reviewboard

People

Description

The code for centering diff collapse buttons has been refactored into the
RB.CenteredElementManager. This view centres a set of elements within
their respective container elements.

Testing Done

Verified the collapsable diff context and diff viewer collapse buttons
still centred correctly.

Issues

Description	From	Last Updated
In your summary/description, you have a variety of spellings of "center". I guess "centring" is maybe Canadian, but the implementation …	david	Sept. 29, 2016, 2:26 p.m.
This function doesn't take any args (anymore?)	david	Sept. 28, 2016, 11:59 a.m.
Doc comment?	david	Sept. 28, 2016, 11:59 a.m.
Doc comment?	david	Sept. 28, 2016, 11:59 a.m.
Doc comment?	david	Sept. 28, 2016, 12:01 p.m.
Doc comment?	david	Sept. 28, 2016, 12:01 p.m.
Doc comment?	david	Sept. 28, 2016, 12:01 p.m.
Trailing comma?	david	Sept. 28, 2016, 12:01 p.m.
Can we call the parameter elements instead of els?	david	Sept. 29, 2016, 2:25 p.m.
There's an extra blank line here.	david	Oct. 6, 2016, 1:06 p.m.

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/ui/views/centeredElementManager.es6.js
    reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
    reviewboard/static/rb/js/views/diffFragmentQueueView.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/ui/views/centeredElementManager.es6.js
    reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
    reviewboard/static/rb/js/views/diffFragmentQueueView.js

reviewboard/static/rb/js/ui/views/centeredElementManager.es6.js (Diff revision 1)

The issue has been resolved. Show all issues

This function doesn't take any args (anymore?)

reviewboard/static/rb/js/ui/views/centeredElementManager.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Doc comment?
```
reviewboard/static/rb/js/ui/views/centeredElementManager.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Doc comment?
```
reviewboard/static/rb/js/ui/views/centeredElementManager.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Doc comment?
```
reviewboard/static/rb/js/ui/views/centeredElementManager.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Doc comment?
```
reviewboard/static/rb/js/ui/views/centeredElementManager.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Doc comment?
```
reviewboard/static/rb/js/ui/views/centeredElementManager.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Trailing comma?
```

Change Summary:

Addressed David's issues.

Commit:

311e1327201118f012c1d6c4939c85315e1af4f6

7077b2f2dc560bf369074fa70a0c5d0484c2064a

Diff:

Revision 2 (+119 -207)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
	reviewboard/static/rb/js/ui/views/centeredElementManager.es6.js
	reviewboard/static/rb/js/views/diffFragmentQueueView.js

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/ui/views/centeredElementManager.es6.js
    reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
    reviewboard/static/rb/js/views/diffFragmentQueueView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/ui/views/centeredElementManager.es6.js
    reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
    reviewboard/static/rb/js/views/diffFragmentQueueView.js

The issue has been resolved. Show all issues

In your summary/description, you have a variety of spellings of "center". I guess "centring" is maybe Canadian, but the implementation in the code uses "centered", and "centeres" just seems incorrect. I'd prefer "centered" / "centers" / "centering".

And now none of those look like words =P

reviewboard/static/rb/js/ui/views/centeredElementManager.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we call the parameter elements instead of els?
```

Change Summary:: De-Canadian-ised (:P) the description and summary. Addressed David's issues.
Summary:: Refactor out the code for centring diff collapse buttons
Refactor out the code for centering diff collapse buttons
Description:: ~
The code for centring diff collapse buttons has been refactored into the
~ RB.CenteredElementManager. This view centeres a set of elements within
~
The code for centering diff collapse buttons has been refactored into the
~ RB.CenteredElementManager. This view centres a set of elements within
their respective container elements.

Change Summary:

Actually publish changes

Commit:

7077b2f2dc560bf369074fa70a0c5d0484c2064a

d1df202430a8c8597dd4a427ab31a1eee64a03e9

Diff:

Revision 3 (+119 -207)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
	reviewboard/static/rb/js/ui/views/centeredElementManager.es6.js
	reviewboard/static/rb/js/views/diffFragmentQueueView.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/ui/views/centeredElementManager.es6.js
    reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
    reviewboard/static/rb/js/views/diffFragmentQueueView.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/ui/views/centeredElementManager.es6.js
    reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
    reviewboard/static/rb/js/views/diffFragmentQueueView.js

Ship it!

reviewboard/static/rb/js/ui/views/centeredElementManager.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
There's an extra blank line here.
```

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

~		The code for centring diff collapse buttons has been refactored into the
~		`RB.CenteredElementManager`. This view centeres a set of elements within
	~	The code for centering diff collapse buttons has been refactored into the
	~	`RB.CenteredElementManager`. This view centres a set of elements within
		their respective container elements.