[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
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
Review request changed
- 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!
-
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
Review request changed
- Change Summary:
-
Putting changes together with some minor fixes. Subview is a proper (if uninteresting) constructor. Basically, my playground's in place.
-
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
Review request changed
- 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
SM
Review request changed
- Change Summary:
-
Cleaning whitespace issues before moving on.
- Diff:
-
Revision 6 (+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
SM
Review request changed
- 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
Review request changed
- 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
Review request changed
- 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
-
-
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.
-
-
-
-
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('')),' -
Same thing as the template above. Also there should be a blank line between these two templates (b/n line 11 and 12).
-
-
-
-
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); }, -
-
-
-
-
-
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 style is that variables should be defined with 1 var. For example:
var furthest, curr, scroll; -
-
-
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
Review request changed
- 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. - Diff:
-
Revision 10 (+280 -30)
-
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
Review request changed
- 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