Add Bookmarking feature to Diff Viewer.
Review Request #7658 — Created Sept. 25, 2015 and updated
Add bookmarking feature in diff viewer so users can create and quickly jump
between bookmarks. Intended to help users moving back and forth between
different parts in the diff viewer when doing code reviews.A "ghost" bookmark will follow the cursor on the right margin of the diff
viewer, which a user can click on to create new bookmarks on the hovered line
of the diff file. Clicking on the created bookmark will scroll the window
to where the bookmarked line is. Bookmarks are saved per diff using browser
Cookie, and will persist for one week.Usability features include assigning different colors to bookmarks for easy
differentiation, positioning bookmarks on where the target line is relative
to the whole page, and animating the position at bookmark creation so users
can see where the newly created bookmark will reside.
Automated JS test cases:
Ghost bookmark
- should move along with the cursor on the diff
- becomes invisible when the cursor is not on diff
- expands when the cursor approaches
- shrinks when the cursor moves away
- creates a new bookmark when clicked on it
- no ghost bookmark when there already is a bookmark on the hovered lineBookmarks
- appears in front of ghost bookmark
- expands when the cursor approaches
- shrinks when the cursor moves away
- will animate on creation
- on file header should only display the filename
- on a line in a file should display the filename and the line number
- on an expanded chunk will hide when the chunk is collapsed
- on a collapsed chunk will appear when the chunk is expanded
- should have different colors (for first 10 bookmarks)
- will be removed when clicked on [x]
- will scroll the page when clicked
- can be created from cookies
- will trigger cookie save event on creation or removalFollowing features are tested manually on local devserver:
Bookmarks
- on an empty line should display nicely
- on a line that wraps on browser as multiple lines should display nicely
- added bookmark will appear again after refreshing on the page
- removed bookmarks will not appear again after refreshing on the page
- should be positioned where the main bookmark is relative to the whole page
- are repositioned when window is resized
- are repositioned when a chunk is expanded/collapsed
- are repositioned when a file is loaded
| Description | From | Last Updated |
|---|---|---|
|
Space after selector. |
 brennie
|
|
|
Alphabetize these. |
brennie
|
|
|
We should set a constant in defs.less for this. e.g. @ghost-bookmark-background-color: rgba(173, 173, 173, 0.5); |
brennie
|
|
|
Can we call this RB.MookmarkModel? |
brennie
|
|
|
There should be a defaults object that contains safe default values (although fileName would probably be null in that case) |
brennie
|
|
|
diffID |
brennie
|
|
|
interdiffID |
brennie
|
|
|
We really don't want to have models stored in models. Backbone doesn't support it that well. Instead, you can pass … |
brennie
|
|
|
This should be in collections/bookmarkCollection.js |
brennie
|
|
|
Likewise, RB.BookmarkCollection ? |
brennie
|
|
|
Missing semicolon. |
brennie
|
|
|
RB.BookmarkContainerView Also, this should be in its own separate file. |
brennie
|
|
|
We use lowerCamelCase for JS names and hyphen-case for HTML names, so this should be bookmark-container. |
brennie
|
|
|
Is this supposed to return something? If not, we do not need the return statement. |
brennie
|
|
|
Please format this like: if (!noRender) { bookmarkerView.render(); } |
brennie
|
|
|
We use one var statement as the first statement in any function. |
brennie
|
|
|
You are likely going to want to use _.throttle here. Listening for mousemoves is expensive. |
brennie
|
|
|
We put a space between the ) and the {. Please fix this throughout. |
 david
|
|
|
Should be bookmark (not Bookmark) |
david
|
|
|
This line isn't necessary. |
david
|
|
|
Variables should start with a lower-case letter. |
david
|
|
|
You don't need the !! here since it's just a truthy-test. All conditionals should also have {}, regardless of whether … |
david
|
|
|
It's probably better to format this with one call per line: this.$ghostBookmark .removeClass('open') .find('.bookmark-fold') .css('width', 0); |
david
|
|
|
This stuff should probably happen in render rather than initialize. |
david
|
|
|
Make sure you're indenting 4 spaces in JavaScript code. |
david
|
|
|
Make sure you're indenting 4 spaces in JavaScript code. |
david
|
|
|
The ?: operator will already convert to a bool, so you don't need !! |
david
|
|
|
You can use _.bind here, too. |
david
|
|
|
This should be the last line in the file. This is a vim modeline which gives the editor settings for … |
brennie
|
|
|
You can nest this in the .close rule with &:hover. |
brennie
|
|
|
You should use .transition(). It does a prefixed version of transition. |
brennie
|
|
|
"the diffviewer" Needs a period. |
brennie
|
|
|
I'm not sure if the cookie data should be passed in via options or not. You should maybe ping Christian … |
brennie
|
|
|
Put these on seperate lines. |
brennie
|
|
|
Remove this comment. |
brennie
|
|
|
We use jquery.cookie. You can use it for reading/writing cookies. Check out the docs |
brennie
|
|
|
Can we call this cookie ? |
brennie
|
|
|
Blank line between these. |
brennie
|
|
|
Remove this comment. |
brennie
|
|
|
Replace this with a for loop. |
brennie
|
|
|
Since we're not actually transmuting the models or options, I feel like we should do this in a signal handler. … |
brennie
|
|
|
Needs a doc comment. |
brennie
|
|
|
Put these on seperate lines, with defined variables first. |
brennie
|
|
|
Can we call this bookmark ? |
brennie
|
|
|
remove this comment. |
brennie
|
|
|
This overwrites all of the cookie that is set, doesn't it? We really should use $.cookie here. |
brennie
|
|
|
Needs a period. |
brennie
|
|
|
Needs a doc comment. |
brennie
|
|
|
Just call it template. That is the convention we use. |
brennie
|
|
|
Space after , between items. |
brennie
|
|
|
Remove this comment. |
brennie
|
|
|
This should use sentence casing, e.g., begin with a capital and end with a period. "from a clone of the" |
brennie
|
|
|
Missing a var statement. e isn't a descriptive variable name; we should use bookmark instead. |
brennie
|
|
|
Since you're doing a filter & map on the values and only using them once, we can just do a … |
brennie
|
|
|
Backbone.Model.get only takes one key at a time, so this should be: e.get('fileDiffID'), e.get('interFileDiffID') |
brennie
|
|
|
If this is private it should be prefixed with an underscore. Also, this needs a doccomment. |
brennie
|
|
|
Remove this line. |
brennie
|
|
|
view is more readable than bview. |
brennie
|
|
|
We format multi-line comments in JS as follows: /* * Line 1. * Line 2. * ... */ |
brennie
|
|
|
You can do view.remove().render() because remove should return the view. |
brennie
|
|
|
Same comment as above regards to public/private-ness and doc comment. |
brennie
|
|
|
Should be prefixed with underscore. Needs doc comment. |
brennie
|
|
|
Should be prefixed with underscore. Needs doc comment. |
brennie
|
|
|
Blank line between these. |
brennie
|
|
|
Doc comment. |
brennie
|
|
|
var $target = $(e.target), fileName, line, top; |
brennie
|
|
|
Blank line between statement and block. |
brennie
|
|
|
See above re: multi-line comments. |
brennie
|
|
|
This is quite hard to read. I'm not sure what nodeType == 3 means? Is there a constnat we can … |
brennie
|
|
|
Should be indented 4 spaces not 2. |
brennie
|
|
|
What do you mean, without updating obj? |
brennie
|
|
|
Sentence casing. |
brennie
|
|
|
} else { |
brennie
|
|
|
doc comment. |
brennie
|
|
|
Doc comment. |
brennie
|
|
|
We prefix jQuery objects, so this should be $diffViewElem. Also, you can shorten Elem to El. |
brennie
|
|
|
Needs doc comment. |
brennie
|
|
|
Should be indented 4. |
brennie
|
|
|
var fileDiff = file.get('filediff'), interFileDiff = file.get('interfilediff'); |
brennie
|
|
|
We can write this more clearly as : return (filediff.id === fileDiffID && ((interFileDiff === null && interFileDiffID === null) … |
brennie
|
|
|
=== |
brennie
|
|
|
Indent multiples of 4 spaces. |
brennie
|
|
|
Missing a period. |
brennie
|
|
|
Missing a period. |
brennie
|
|
|
Col: 27 W601 .has_key() is deprecated, use 'in' |
 reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Can we rename the cookie to "diff-bookmarks" ? Or maybe we should just have it be "bookmarks" (as we may … |
 chipx86
|
|
|
Just some tweaks to this. The argument name should include the type, and for request (a very common argument), we … |
chipx86
|
|
|
This will be faster as: bookmark_cookie = request.COOKIES.get('diffbookmarks', '') |
chipx86
|
|
|
We namespace these names, so these would be best as @bookmark-ghost-bg-color and @bookmark-default-bg-color, in alphabetical order. |
chipx86
|
|
|
"Bookmarks" |
chipx86
|
|
|
I know we don't have this in many places, but I'm trying to get in the habit: Can you add … |
chipx86
|
|
|
We always use the long form: #FFFF00. Should also have a constant. |
chipx86
|
|
|
All z-indexes must be registered in defs.less as constants. |
chipx86
|
|
|
Not sure what e() is, but elsewhere, we do: .transition(padding-left 0.2s linear, top 0.2s ease-out;); (Note the inner ; near … |
chipx86
|
|
|
We use the form of: 1px #333 solid This also needs a constant defined. Same with all other colors. |
chipx86
|
|
|
All repeating sizes for padding, fonts, etc. should have a constant. |
chipx86
|
|
|
Here and elsewhere, make sure there's a space before {. |
chipx86
|
|
|
Selector groups are always in this order: & { ... } &#id { ... } &:pseudo-class { ... } &.class-name … |
chipx86
|
|
|
No capitalization of "bookmarks" needed. Can you have this go into detail on what the class does? What it's tracking, … |
chipx86
|
|
|
Multi-line comments must always be in this form: /* * line * line */ Same with other comments below. |
chipx86
|
|
|
Function/method docs must be in this form: /* * One-line summary. * * Multi-line description. */ When there are arguments … |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
Space before {. |
chipx86
|
|
|
Let's pull out model.get('fileDiffID') just once. |
chipx86
|
|
|
This would be better named as "updatedCookie". Make sure this is documented in the method docs. |
chipx86
|
|
|
No blank line here. |
chipx86
|
|
|
We only use one var statement per function, with one variable per line, like: var collection, listenerObj; |
chipx86
|
|
|
I don't know that this is really needed. If you just need to check that the event emitted properly, you … |
chipx86
|
|
|
Space before {. No need for return;, since that's implied. |
chipx86
|
|
|
Ideally, you should use .toBe instead of .toEqual in tests. The reason is that .toBe is equivalent to strict JavaScript … |
chipx86
|
|
|
Can you add docs for each of these? |
chipx86
|
|
|
Just like methods, this should be in the form of: /* * One-line summary. * * Multi-line description. */ This … |
chipx86
|
|
|
This is the default, so you don't need this here. |
chipx86
|
|
|
When named template, it implies it's the template for the view itself. You probably want to call this ghostBookmarkTemplate. |
chipx86
|
|
|
The strings must align. Indentation should be within the string, 1 space. |
chipx86
|
|
|
Each method needs a docstring. |
chipx86
|
|
|
Element-related initialization should go in render. |
chipx86
|
|
|
Any member variables that don't need public access should be made private (prefix the name with a _). |
chipx86
|
|
|
render must return this at the end. |
chipx86
|
|
|
render shouldn't be responsible for deciding where its main element is put. That's the responsibility of the caller, only. |
chipx86
|
|
|
These should instead go in events, like: events: { 'click .ghost': '_onGhostBookmarkClicked', ... } |
chipx86
|
|
|
That sounds odd to me, but you should definitely be doing: return _super(this).remove.call(this); instead of removing it manually. The parent … |
chipx86
|
|
|
Can you tell me what the body query is about? Is this about whether it's currently packed into the page … |
chipx86
|
|
|
What's this for? Ideally, you wouldn't need to do this. I want to be sure I understand it well. |
chipx86
|
|
|
The list of variables should begin with defined variables, so: var cookie = JSON.parse(cookieString), bookmark; |
chipx86
|
|
|
i must be declared above, in the existing var list. |
chipx86
|
|
|
All jQuery-wrapped elememnts should be named like $diffReviewable. |
chipx86
|
|
|
This is a bit confusing. Can you instead just do this work when binding the element? If you're just looking … |
chipx86
|
|
|
One var statement. |
chipx86
|
|
|
.parents() is very expensive. Is there any way we can be smarter about what we're looing up? Can you explain … |
chipx86
|
|
|
Blank line after this. |
chipx86
|
|
|
This can be one statement, and you can probably use .width(). How about: var $bookmarkText = this._$ghostBookmark.find('.bookmark-text'); this._$ghostBookmark.find('.bookmark-fold') .width($bookmarkText.outerWidth()); Also, … |
chipx86
|
|
|
Sentence capitalization and period. |
chipx86
|
|
|
The .css should be indented one. |
chipx86
|
|
|
These should go first, since they're defined. |
chipx86
|
|
|
.parents() is very expensive, so we shoud minimize this however we can. Certainly, we should never call it more than … |
chipx86
|
|
|
This is very awkwardly formatted. can we pull some of this out somehow? |
chipx86
|
|
|
This has already been pulled out of diffModel. |
chipx86
|
|
|
Only one blank line here. |
chipx86
|
|
|
Space before {. |
chipx86
|
|
|
filename +=. |
chipx86
|
|
|
You can chain these. |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
Values should align. |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
Needs docs. |
chipx86
|
|
|
Same comments as above with the formatting. |
chipx86
|
|
|
This will cause problems. You need to instead do: this.$el.html(this.template()); Also, this must happen in render(). |
chipx86
|
|
|
Anything that isn't public API should have a _ in front of it. |
chipx86
|
|
|
Blank line before comments. |
chipx86
|
|
|
Would be best to pull these out into variables, since other things seem to use these. |
chipx86
|
|
|
Blank lines before comments. |
chipx86
|
|
|
If it's just one property, do: this.$el.css('top', topPosition); |
chipx86
|
|
|
Blank line between these. vars and returns generally should be thought of as their own "paragraphs" of code. |
chipx86
|
|
|
Parameters must align. |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
Need a space before {. |
chipx86
|
|
|
Because of how JavaScript works, this can pick up data you don't expect. You need to use this form: for … |
chipx86
|
|
|
Best to only pull this out once. |
chipx86
|
|
|
Indentation is wrong. !== should be used instead of !=. |
chipx86
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Should ideally be in alphabetical order. |
chipx86
|
|
|
Here too. |
chipx86
|
|
|
And these. |
chipx86
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
-
-
-
-
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 1) We should set a constant in
defs.lessfor this.e.g.
@ghost-bookmark-background-color: rgba(173, 173, 173, 0.5); -
reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js (Diff revision 1) Can we call this
RB.MookmarkModel? -
reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js (Diff revision 1) There should be a
defaultsobject that contains safe default values (althoughfileNamewould probably benullin that case) -
-
-
reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js (Diff revision 1) We really don't want to have models stored in models. Backbone doesn't support it that well. Instead, you can pass
diffModelinoptions, e.g.var bookmarkModel = new RB.BookmarkerModel({ diffModel: diffModel });and then access it either through
this.optionsoroptions(which is passed as the first argument to initialize). -
reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js (Diff revision 1) This should be in
collections/bookmarkCollection.js -
reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js (Diff revision 1) Likewise,
RB.BookmarkCollection? -
-
reviewboard/static/rb/js/diffviewer/views/bookmarkerView.js (Diff revision 1) RB.BookmarkContainerViewAlso, this should be in its own separate file.
-
reviewboard/static/rb/js/diffviewer/views/bookmarkerView.js (Diff revision 1) We use
lowerCamelCasefor JS names andhyphen-casefor HTML names, so this should bebookmark-container. -
reviewboard/static/rb/js/diffviewer/views/bookmarkerView.js (Diff revision 1) Is this supposed to return something? If not, we do not need the return statement.
-
reviewboard/static/rb/js/diffviewer/views/bookmarkerView.js (Diff revision 1) Please format this like:
if (!noRender) { bookmarkerView.render(); } -
reviewboard/static/rb/js/diffviewer/views/bookmarkerView.js (Diff revision 1) We use one
varstatement as the first statement in any function. -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1) You are likely going to want to use
_.throttlehere. Listening for mousemoves is expensive.
YN
Change Summary:
WIP ver.0.2: Ghost bookmarker shrinking and expanding.
Description: |
|
||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+335) |
-
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/css/defs.less reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js reviewboard/static/rb/js/diffviewer/views/bookmarkView.js reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/css/defs.less reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js reviewboard/static/rb/js/diffviewer/views/bookmarkView.js reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js
YN
Change Summary:
Updated description with the most recent commit on styling thumbnail and actual bookmarks
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+406) |
-
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/css/defs.less reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js reviewboard/static/rb/js/diffviewer/views/bookmarkView.js reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/css/defs.less reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js reviewboard/static/rb/js/diffviewer/views/bookmarkView.js reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js
-
So I don't have a lot of comments on the code yet, since it looks like you're still in early exploration.
However, I do want to help guide you into our coding style now, before you write too much code. These comments are primarily about that.
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 3) We put a space between the
)and the{. Please fix this throughout. -
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 3) Should be
bookmark(notBookmark) -
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 3) This line isn't necessary.
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 3) Variables should start with a lower-case letter.
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 3) You don't need the
!!here since it's just a truthy-test.All conditionals should also have {}, regardless of whether the block therein is only one line.
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 3) It's probably better to format this with one call per line:
this.$ghostBookmark .removeClass('open') .find('.bookmark-fold') .css('width', 0); -
reviewboard/static/rb/js/diffviewer/views/bookmarkView.js (Diff revision 3) This stuff should probably happen in
renderrather thaninitialize. -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 3) Make sure you're indenting 4 spaces in JavaScript code.
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 3) Make sure you're indenting 4 spaces in JavaScript code.
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 3) The
?:operator will already convert to a bool, so you don't need!! -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 3) You can use
_.bindhere, too.
YN
Change Summary:
Updated code (Cookie support, thumbnail shrink/expand), added screen shots
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+481) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/css/defs.less reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js reviewboard/static/rb/js/diffviewer/views/bookmarkView.js reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/css/defs.less reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js reviewboard/static/rb/js/diffviewer/views/bookmarkView.js reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js
YN
Change Summary:
Updated with bugfix commits and random bookmark coloring. Also added test cases.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 5 (+520) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/css/defs.less reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js reviewboard/static/rb/js/diffviewer/views/bookmarkView.js reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/css/defs.less reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js reviewboard/static/rb/js/diffviewer/views/bookmarkView.js reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js
YN
Change Summary:
Added initial draft (+mock image) of usability improvements including unified bookmark representation.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+533) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
-
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/css/defs.less reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js reviewboard/static/rb/js/diffviewer/views/bookmarkView.js reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/css/defs.less reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js reviewboard/static/rb/js/diffviewer/views/bookmarkView.js reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js
-
This is looking good and a lot of these issues are style nits. However, I have some general comments:
- You should use sentence casing for all comments. That means they should begin with a capital and end with a period.
- You should prefer
===over==unless you are intentionally doing a type-coercing comparision. In that case, you should document why you are using coercion over expicit equality. - Your functions need doc comments unless they're overriding functions like
render,initialize, etc.
-
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 6) This should be the last line in the file.
This is a vim modeline which gives the editor settings for editing the file. Vim expects it to occur at the end of the file.
-
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 6) You can nest this in the
.closerule with&:hover. -
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 6) You should use
.transition(). It does a prefixed version oftransition. -
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6) "the diffviewer"
Needs a period.
-
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6) I'm not sure if the cookie data should be passed in via
optionsor not. You should maybe ping Christian and ask him. -
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6) Put these on seperate lines.
-
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6) Remove this comment.
-
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6) We use
jquery.cookie. You can use it for reading/writing cookies. Check out the docs -
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6) Can we call this
cookie? -
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6) Blank line between these.
-
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6) Remove this comment.
-
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6) Replace this with a
forloop. -
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6) Since we're not actually transmuting the models or options, I feel like we should do this in a signal handler.
In initialize we can do:
this.listenTo(this, 'update', this.saveToCookie);That way,
saveToCookiewill be called wheneveradd/remove/setis called and we don't need to define these functions.(Just make sure this is in initialize after the cookie data is loaded).
-
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6) Needs a doc comment.
-
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6) Put these on seperate lines, with defined variables first.
-
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6) Can we call this
bookmark? -
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6) remove this comment.
-
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6) This overwrites all of the cookie that is set, doesn't it? We really should use
$.cookiehere. -
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) Needs a doc comment.
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) Just call it
template. That is the convention we use. -
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) Space after
,between items. -
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) Remove this comment.
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) This should use sentence casing, e.g., begin with a capital and end with a period.
"from a clone of the"
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) Missing a
varstatement.eisn't a descriptive variable name; we should usebookmarkinstead. -
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) Since you're doing a filter & map on the values and only using them once, we can just do a map and do the filtering inline as an if:
this.collection.each(function(bookmark) { var fileDiffID = bookmark.get('fileDiffID'), interFileDiffID = bookmark.get('interFileDiffID'); if (this.pageView.showsDiffFile(fileDiffID, interFileDiffID)) { this.createBookMarkView(bookmark, true); } }, this);This will be more efficient becuase there will be
O(n)functions calls instead of ~2n. -
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) Backbone.Model.getonly takes one key at a time, so this should be:e.get('fileDiffID'), e.get('interFileDiffID') -
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) If this is private it should be prefixed with an underscore.
Also, this needs a doccomment.
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) Remove this line.
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) viewis more readable thanbview. -
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) We format multi-line comments in JS as follows:
/* * Line 1. * Line 2. * ... */ -
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) You can do
view.remove().render()becauseremoveshould return the view. -
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) Same comment as above regards to public/private-ness and doc comment.
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) Should be prefixed with underscore. Needs doc comment.
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) Should be prefixed with underscore. Needs doc comment.
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) Blank line between these.
-
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) var $target = $(e.target), fileName, line, top; -
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) Blank line between statement and block.
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) See above re: multi-line comments.
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) This is quite hard to read.
I'm not sure what
nodeType == 3means? Is there a constnat we can use.Really, I'm not sure what this is doing in general. Ping me in slack and we can discuss this.
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) Should be indented 4 spaces not 2.
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) What do you mean, without updating obj?
-
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6) Sentence casing.
-
-
-
-
reviewboard/static/rb/js/diffviewer/views/bookmarkView.js (Diff revision 6) We prefix jQuery objects, so this should be
$diffViewElem.Also, you can shorten
ElemtoEl. -
-
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 6) var fileDiff = file.get('filediff'), interFileDiff = file.get('interfilediff'); -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 6) We can write this more clearly as :
return (filediff.id === fileDiffID && ((interFileDiff === null && interFileDiffID === null) || interFileDiff.id === interFileDiffID));This should use
===not==. -
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 6) Indent multiples of 4 spaces.
-
-
YN
Change Summary:
Animation at bookmark creation, moving cookie management to UserSession, code styles.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+632) |
-
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js
YN
Change Summary:
Updated with rewritten cookie management and other code style cleanups.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+676) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js -
YN
Change Summary:
Bug and code style fixes, start adding automated tests.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 9 (+987) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js -
YN
Change Summary:
Add JS test cases, Update description and testing done.
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 10 (+1302) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js -
YN
Change Summary:
Fix bug with loaded cookie not being up to date due to etag identification
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js -
-
First off, this feature is awesome and boy do I wish I had it while reviewing this change :)
Second, I have a lot of comments here. The good news is, most of these are stylistic things that I repeat over and over. You'll be able to plow through most of these.
However, I did not highlight every occurrence, so I'll want you to go through and find, for instance, cases where you have
function(){instead offunction() {, and comments that need a blank line before them, etc.There also needs to be more documentation. We're working on fleshing these out, and we want them to follow the Python standards. I'm happy to answer questions on this.
I also have some implementation comments, and comments about performance. For instance, I point out several cases where
.parents()is used, which is an expensive function that should largely be avoided if at all possible, so I'd like to work with you to better understand these and see if we can come up with a better method. -
reviewboard/diffviewer/views.py (Diff revision 11) Can we rename the cookie to "diff-bookmarks" ? Or maybe we should just have it be "bookmarks" (as we may later want to port this to text-based file attachments, perhaps).
It's also cheaper to fetch the data out, and set the header if that data is not
None. Further cheaper if you assume it exists in atry/except, like:try: response['X-ReviewBoard-Bookmarks'] = request.COOKIES['bookmarks'] except KeyError: # There aren't any bookmark cookies set. Ignore. -
reviewboard/diffviewer/views.py (Diff revision 11) Just some tweaks to this. The argument name should include the type, and for
request(a very common argument), we don't need to go into details of how it'll be used. It just runs the risk of going stale as we use more of it down the road.request (django.http.HttpRequest): The HTTP request from the client. -
reviewboard/diffviewer/views.py (Diff revision 11) This will be faster as:
bookmark_cookie = request.COOKIES.get('diffbookmarks', '') -
reviewboard/static/rb/css/defs.less (Diff revision 11) We namespace these names, so these would be best as
@bookmark-ghost-bg-colorand@bookmark-default-bg-color, in alphabetical order. -
-
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) I know we don't have this in many places, but I'm trying to get in the habit: Can you add a comment for each of these selectors documenting what part of the UI this is?
-
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) We always use the long form:
#FFFF00. Should also have a constant. -
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) All z-indexes must be registered in
defs.lessas constants. -
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) Not sure what
e()is, but elsewhere, we do:.transition(padding-left 0.2s linear, top 0.2s ease-out;);(Note the inner
;near the end. This forces LessCSS to consider our commas as literal text, and not argument separators.) -
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) We use the form of:
1px #333 solidThis also needs a constant defined. Same with all other colors.
-
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) All repeating sizes for padding, fonts, etc. should have a constant.
-
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) Here and elsewhere, make sure there's a space before
{. -
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11) Selector groups are always in this order:
& { ... } &#id { ... } &:pseudo-class { ... } &.class-name { ...} element { ... } #id { ... } .class-name { ... }Those should be in alphabetical order within a grouping.
This applies to all the CSS in this change.
-
reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js (Diff revision 11) No capitalization of "bookmarks" needed.
Can you have this go into detail on what the class does? What it's tracking, how it's used, etc.
-
reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js (Diff revision 11) Multi-line comments must always be in this form:
/* * line * line */Same with other comments below.
-
reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js (Diff revision 11) Function/method docs must be in this form:
/* * One-line summary. * * Multi-line description. */When there are arguments or return values, add:
/* * ... * * Args: * bookmark (the type): * The description. * * next_argument (type): * ... * * Returns: * the return type: * The description. */Same with all other methods.
-
reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js (Diff revision 11) Blank line between these.
-
reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js (Diff revision 11) Blank line between these.
-
reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js (Diff revision 11) Space before
{. -
reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js (Diff revision 11) Let's pull out
model.get('fileDiffID')just once. -
reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js (Diff revision 11) This would be better named as "updatedCookie".
Make sure this is documented in the method docs.
-
reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js (Diff revision 11) No blank line here.
-
reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js (Diff revision 11) We only use one
varstatement per function, with one variable per line, like:var collection, listenerObj; -
reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js (Diff revision 11) I don't know that this is really needed. If you just need to check that the event emitted properly, you should spy on
ollection.triggerand check that it was called with the expected parameters for emitting that event. -
reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js (Diff revision 11) Space before
{.No need for
return;, since that's implied. -
reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js (Diff revision 11) Ideally, you should use
.toBeinstead of.toEqualin tests. The reason is that.toBeis equivalent to strict JavaScript comparison (===), which means that1 === 1but1 !== "1".If you use
.toEqual, it's like==, so1 == "1".Unit tests should be strict, so we want to catch future issues like the possibility of an integer turning into a string.
-
reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js (Diff revision 11) Can you add docs for each of these?
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) Just like methods, this should be in the form of:
/* * One-line summary. * * Multi-line description. */This should also go into more detail of what this class is in charge of doing.
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) This is the default, so you don't need this here.
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) When named
template, it implies it's the template for the view itself. You probably want to call thisghostBookmarkTemplate. -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) The strings must align. Indentation should be within the string, 1 space.
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) Each method needs a docstring.
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) Element-related initialization should go in
render. -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) Any member variables that don't need public access should be made private (prefix the name with a
_). -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) rendermust returnthisat the end. -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) rendershouldn't be responsible for deciding where its main element is put. That's the responsibility of the caller, only. -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) These should instead go in
events, like:events: { 'click .ghost': '_onGhostBookmarkClicked', ... } -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) That sounds odd to me, but you should definitely be doing:
return _super(this).remove.call(this);instead of removing it manually. The parent
remove()does more than this. -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) Can you tell me what the
bodyquery is about? Is this about whether it's currently packed into the page somewhere? -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) What's this for? Ideally, you wouldn't need to do this. I want to be sure I understand it well.
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) The list of variables should begin with defined variables, so:
var cookie = JSON.parse(cookieString), bookmark; -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) imust be declared above, in the existingvarlist. -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) All jQuery-wrapped elememnts should be named like
$diffReviewable. -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) This is a bit confusing. Can you instead just do this work when binding the element? If you're just looking to bind additional arguments, you can pass them to
_.bind(). -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) One
varstatement. -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) .parents()is very expensive. Is there any way we can be smarter about what we're looing up? Can you explain these checks in more detail to me?I'm wondering if
hasClass()would be enough. -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) Blank line after this.
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) This can be one statement, and you can probably use
.width(). How about:var $bookmarkText = this._$ghostBookmark.find('.bookmark-text'); this._$ghostBookmark.find('.bookmark-fold') .width($bookmarkText.outerWidth());Also, how about pulling out things like
.bookmark-textinrender()above, so that we don't have to keep re-fetching? -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) Sentence capitalization and period.
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) The
.cssshould be indented one. -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) These should go first, since they're defined.
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) .parents()is very expensive, so we shoud minimize this however we can. Certainly, we should never call it more than once for the same element with the same selector. -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) This is very awkwardly formatted. can we pull some of this out somehow?
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) This has already been pulled out of
diffModel. -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) Only one blank line here.
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) Space before
{. -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) filename +=. -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) You can chain these.
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) Blank line between these.
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) Values should align.
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11) Blank line between these.
-
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js (Diff revision 11) Same comments as above with the formatting.
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js (Diff revision 11) This will cause problems. You need to instead do:
this.$el.html(this.template());Also, this must happen in
render(). -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js (Diff revision 11) Anything that isn't public API should have a
_in front of it. -
reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js (Diff revision 11) Blank line before comments.
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js (Diff revision 11) Would be best to pull these out into variables, since other things seem to use these.
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js (Diff revision 11) Blank lines before comments.
-
reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js (Diff revision 11) If it's just one property, do:
this.$el.css('top', topPosition); -
reviewboard/static/rb/js/models/userSessionModel.js (Diff revision 11) Blank line between these.
vars andreturns generally should be thought of as their own "paragraphs" of code. -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11) Parameters must align.
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11) Blank line between these.
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11) Need a space before
{. -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11) Because of how JavaScript works, this can pick up data you don't expect. You need to use this form:
for (var fileID in data) { if (data.hasOwnProperty(fileID)) { ... } }That assumes that
datais an object and not an array.Also, space before
{. -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11) Best to only pull this out once.
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11) Indentation is wrong.
!==should be used instead of!=. -
-
-
YN
Change Summary:
More documentations, coding styles, few small performance optimizations. Documentations and other polishes still wip.
Diff: |
Revision 12 (+1491 -4)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js -
YN
Change Summary:
Fixes for coding styles, readability, documentations and micro-optimizations.
Diff: |
Revision 13 (+1593 -4)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js -
YN
Change Summary:
Add forgotten documentations for diffViewerPageView and userSessionModel.
Diff: |
Revision 14 (+1622 -4)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js -
YN
Change Summary:
Add forgotten spaces between parensthesis and bracket.
Diff: |
Revision 15 (+1622 -4)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/models/userSessionModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js -
Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/bookmarkerView.js reviewboard/static/rb/css/pages/diffviewer.less Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/views/bookmarkerView.js reviewboard/static/rb/css/pages/diffviewer.less