[WIP] Templating and CSS changes preparing to include a new subview in the draft review banner.
Review Request #8002 — Created Feb. 26, 2016 and discarded
| Information | |
|---|---|
| smith | |
| Review Board | |
| master | |
| Reviewers | |
| reviewboard | |
Templating and CSS changes preparing to include a new subview in the draft review banner.
| Description | From | Last Updated |
|---|---|---|
|
Mixed tabs and spaces. |
 brennie
|
|
|
Mixed tabs and spaces. |
brennie
|
|
|
This file is indendted with tabs and spaces. |
brennie
|
|
|
Trailing whitespace |
brennie
|
|
|
Mixed tabs and spaces. |
brennie
|
|
|
This should be indented two spaces, not two tabs. |
brennie
|
|
|
Can you fix the alignment here? This line should be on the same level as the one above it. The … |
 imadueme
|
|
|
Can you fix the alignment here too? (Maybe the mixed tabs and spaces issue came back?) |
imadueme
|
|
|
Same thing. (Maybe the mixed tabs and spaces issue came back?) |
imadueme
|
|
|
Trailing whitespace (came back?) |
imadueme
|
|
|
Indent template items 4 spaces and have closing ] be on same level as line 5. This is how all … |
imadueme
|
|
|
Same thing as the template above. Also there should be a blank line between these two templates (b/n line 11 … |
imadueme
|
|
|
Space before {. (Super nitpicky I know, but, this is the issue I always get on my stuff :P). |
imadueme
|
|
|
Indent 4 spaces in. |
imadueme
|
|
|
Space before { |
imadueme
|
|
|
Fix indentation. Spaces before {. Use single-quotes instad of double-quotes for strings in js. Space after comma before 2nd 'this'. … |
imadueme
|
|
|
Space before { |
imadueme
|
|
|
Do or do not. There is no try. (Remove the comment) |
imadueme
|
|
|
Remove space before the 2nd 'this'. |
imadueme
|
|
|
This is indented too much. |
imadueme
|
|
|
Space before {. (Find all the others and fix them). |
imadueme
|
|
|
Are you checking for exactly null? If so use !==. Otherwise an undefined might fail this (which is probably not … |
imadueme
|
|
|
Fix indentations and use single quotes. |
imadueme
|
|
|
Fix indentations and use single quotes. |
imadueme
|
|
|
Reviewboard style is that variables should be defined with 1 var. For example: var furthest, curr, scroll; |
imadueme
|
|
|
Mix tabs and spaces again? |
imadueme
|
|
|
Fix indentations. Space before {. Last } should be on it's own line. |
imadueme
|
|
|
All of this should be indented 1 more space in since you added a new div. You indented the closing … |
imadueme
|
SM
Change Summary:
Playing around with a subview-to-be here. But, even in its vestigial form, it won't instantiate, giving a 'RB.BannerDiffListView is not a constructor' error!
Diff: |
Revision 2 (+28) |
|---|
-
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/BannerDiffListView.js reviewboard/static/rb/js/views/draftReviewBannerView.js Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/BannerDiffListView.js reviewboard/static/rb/js/views/draftReviewBannerView.js
-
Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/bannerDiffListView.js reviewboard/static/rb/js/views/draftReviewBannerView.js Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/bannerDiffListView.js reviewboard/static/rb/js/views/draftReviewBannerView.js
SM
Change Summary:
Putting changes together with some minor fixes. Subview is a proper (if uninteresting) constructor. Basically, my playground's in place.
Diff: |
Revision 4 (+36) |
|---|
-
Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/draftReviewBannerView.js reviewboard/templates/reviews/reviewable_base.html reviewboard/static/rb/js/views/bannerDiffListView.js reviewboard/static/rb/css/pages/reviews.less Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/draftReviewBannerView.js reviewboard/templates/reviews/reviewable_base.html reviewboard/static/rb/js/views/bannerDiffListView.js reviewboard/static/rb/css/pages/reviews.less
SM
Change Summary:
Includes the call chain that gets the collection object through to the subview.
Diff: |
Revision 5 (+56 -1) |
|---|
-
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/draftReviewBannerView.js reviewboard/templates/reviews/reviewable_base.html reviewboard/static/rb/js/views/bannerDiffListView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/draftReviewBannerView.js reviewboard/templates/reviews/reviewable_base.html reviewboard/static/rb/js/views/bannerDiffListView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js
-
-
-
-
reviewboard/static/rb/js/views/bannerDiffListView.js (Diff revision 5) This file is indendted with tabs and spaces.
-
-
-
reviewboard/templates/reviews/reviewable_base.html (Diff revision 5) This should be indented two spaces, not two tabs.
-
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/draftReviewBannerView.js reviewboard/templates/reviews/reviewable_base.html reviewboard/static/rb/js/views/bannerDiffListView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/draftReviewBannerView.js reviewboard/templates/reviews/reviewable_base.html reviewboard/static/rb/js/views/bannerDiffListView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js
SM
Change Summary:
Blocked due to scope confusion - I don't have access to the template in the setCollection function (under this._item_template) despite the fact that we have access to it in a really, really similar function on line 73 here: https://github.com/reviewboard/reviewboard/blob/8d663cdc1f0ca11650610548b35752673bc29149/reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js So what's the difference between here and there?
Diff: |
Revision 7 (+63 -1) |
|---|
-
Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/draftReviewBannerView.js reviewboard/templates/reviews/reviewable_base.html reviewboard/static/rb/js/views/bannerDiffListView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/draftReviewBannerView.js reviewboard/templates/reviews/reviewable_base.html reviewboard/static/rb/js/views/bannerDiffListView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js
SM
Change Summary:
Now contains a list and tracks which file has been scrolled over - ie, the tech from the mockup!
There are some obvious display issues going on in this, but I posted a WIP as I clean them up. (Hides when not being scrolled + might be reading the wrong collection? Something to look into.)
Diff: |
Revision 8 (+100 -1) |
|---|
-
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/draftReviewBannerView.js reviewboard/templates/reviews/reviewable_base.html reviewboard/static/rb/js/views/bannerDiffListView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/draftReviewBannerView.js reviewboard/templates/reviews/reviewable_base.html reviewboard/static/rb/js/views/bannerDiffListView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js
SM
Change Summary:
This fixes a bug involving the "display only when scrolled down" behavior of the diff list, making it behave appropriately.
Diff: |
Revision 9 (+101 -1) |
|---|
-
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/draftReviewBannerView.js reviewboard/templates/reviews/reviewable_base.html reviewboard/static/rb/js/views/bannerDiffListView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/draftReviewBannerView.js reviewboard/templates/reviews/reviewable_base.html reviewboard/static/rb/js/views/bannerDiffListView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/pages/views/diffViewerPageView.js
-
-
reviewboard/static/rb/css/pages/reviews.less (Diff revision 9) Can you fix the alignment here? This line should be on the same level as the one above it. The body of this block should only be indented 2 spaces in.
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 9) Can you fix the alignment here too? (Maybe the mixed tabs and spaces issue came back?)
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 9) Same thing. (Maybe the mixed tabs and spaces issue came back?)
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 9) Trailing whitespace (came back?)
-
reviewboard/static/rb/js/views/bannerDiffListView.js (Diff revision 9) Indent template items 4 spaces and have closing
]be on same level as line 5. This is how all templates in the codebase look. It should look like:difflist_template: _.template([ '<div class=\"banner\">', ' Banner is over: <span id=\"bancontent\"></span>', ' <ul id=\"fileList">', ' </ul>', '</div>' ].join('')),' -
reviewboard/static/rb/js/views/bannerDiffListView.js (Diff revision 9) Same thing as the template above. Also there should be a blank line between these two templates (b/n line 11 and 12).
-
reviewboard/static/rb/js/views/bannerDiffListView.js (Diff revision 9) Space before {. (Super nitpicky I know, but, this is the issue I always get on my stuff :P).
-
-
-
reviewboard/static/rb/js/views/bannerDiffListView.js (Diff revision 9) Fix indentation. Spaces before {. Use single-quotes instad of double-quotes for strings in js. Space after comma before 2nd 'this'. Should look like:
setCollection: function(collection) { collection.each(function (file) { $('ul#fileList').append(this._item_template(file.attributes)); }, this); }, -
-
reviewboard/static/rb/js/views/bannerDiffListView.js (Diff revision 9) Do or do not. There is no try. (Remove the comment)
-
reviewboard/static/rb/js/views/bannerDiffListView.js (Diff revision 9) Remove space before the 2nd 'this'.
-
-
reviewboard/static/rb/js/views/bannerDiffListView.js (Diff revision 9) Space before {. (Find all the others and fix them).
-
reviewboard/static/rb/js/views/bannerDiffListView.js (Diff revision 9) Are you checking for exactly null? If so use !==. Otherwise an
undefinedmight fail this (which is probably not a big deal I guess). Better yet use underscore:!(_.isNull(event.data.topmost())) -
reviewboard/static/rb/js/views/bannerDiffListView.js (Diff revision 9) Fix indentations and use single quotes.
-
reviewboard/static/rb/js/views/bannerDiffListView.js (Diff revision 9) Fix indentations and use single quotes.
-
reviewboard/static/rb/js/views/bannerDiffListView.js (Diff revision 9) Reviewboard style is that variables should be defined with 1 var. For example:
var furthest, curr, scroll; -
reviewboard/static/rb/js/views/draftReviewBannerView.js (Diff revision 9) Mix tabs and spaces again?
-
reviewboard/static/rb/js/views/draftReviewBannerView.js (Diff revision 9) Fix indentations. Space before {. Last } should be on it's own line.
-
reviewboard/templates/reviews/reviewable_base.html (Diff revision 9) All of this should be indented 1 more space in since you added a new div. You indented the closing tag for that div correctly as reference. (Since this stuff is nested under the 'banner' div)
SM
Change Summary:
Functionality of existing draft review banner broken down into a subview.
Currently in a broken state - going to be fixing behavior from now on.
I think all the structure is there, though.
-
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/draftReviewBannerView.js reviewboard/static/rb/js/views/bannerDiffListView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/reviewable_base.html reviewboard/static/rb/js/views/reviewBoxView.js reviewboard/static/rb/js/views/bannerDraftNotice.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/draftReviewBannerView.js reviewboard/static/rb/js/views/bannerDiffListView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/reviewable_base.html reviewboard/static/rb/js/views/reviewBoxView.js reviewboard/static/rb/js/views/bannerDraftNotice.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js
SM
Change Summary:
Gosh, this one's a mess!
Alright, the logic in the replyNotice, I think, is correct.
And the bannerDiffList does indeed notify you as to which file you're currently looking at.However, I think I was confused about how other things interact with the banner...
...so it's in a mess on that front.I think it'll probably be a good idea to raze my changes on components other than the banner and its two new subviews to the ground and start anew after the next three days.
Diff: |
Revision 11 (+281 -44)
|
|---|
-
Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/draftReviewBannerView.js reviewboard/static/rb/js/views/bannerDiffListView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/reviewable_base.html reviewboard/static/rb/js/views/reviewBoxView.js reviewboard/static/rb/js/views/bannerDraftNotice.js reviewboard/static/rb/js/pages/views/reviewablePageView.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/views/draftReviewBannerView.js reviewboard/static/rb/js/views/bannerDiffListView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/reviewable_base.html reviewboard/static/rb/js/views/reviewBoxView.js reviewboard/static/rb/js/views/bannerDraftNotice.js reviewboard/static/rb/js/pages/views/reviewablePageView.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js
Tool: Pyflakes Ignored Files: reviewboard/templates/reviews/reviewable_base.html reviewboard/static/rb/css/pages/reviews.less Tool: PEP8 Style Checker Ignored Files: reviewboard/templates/reviews/reviewable_base.html reviewboard/static/rb/css/pages/reviews.less