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
|
-
-
-
-
We should set a constant in
defs.lessfor this.e.g.
@ghost-bookmark-background-color: rgba(173, 173, 173, 0.5); -
-
There should be a
defaultsobject that contains safe default values (althoughfileNamewould probably benullin that case) -
-
-
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). -
-
-
-
-
We use
lowerCamelCasefor JS names andhyphen-casefor HTML names, so this should bebookmark-container. -
-
-
-
YN
- Change Summary:
-
WIP ver.0.2: Ghost bookmarker shrinking and expanding.
- Description:
-
Add Bookmarking feature in Diff viewer so users can
create and quickly jump between bookmarkers. Should help users moving back and forth between different parts of diffs when doing code reviews. 9/25 Starter code for the feature, with ghost bookmarker
chasing cursor and displaying which line in which file it is looking at. + + 10/3 Ghost bookmarker will now shrink and expand in
+ renponse to cursor position. Clicking on the + ghost bookmarker will add bookmark DOM to the page. - 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:
-
Add Bookmarking feature in Diff viewer so users can
create and quickly jump between bookmarkers. Should help users moving back and forth between different parts of diffs when doing code reviews. 9/25 Starter code for the feature, with ghost bookmarker
chasing cursor and displaying which line in which file it is looking at. 10/3 Ghost bookmarker will now shrink and expand in
renponse to cursor position. Clicking on the ghost bookmarker will add bookmark DOM to the page. + + 10/10 Added styling for the actual bookmark as well as the
+ thumbnail. The thumbnail bookmark will appear in the position + relative to the bookmark's position within the entire page, + and will jump to where the bookmark is when clicked on it. - 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.
-
-
-
-
-
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.
-
It's probably better to format this with one call per line:
this.$ghostBookmark .removeClass('open') .find('.bookmark-fold') .css('width', 0); -
-
-
-
-
YN
- Change Summary:
-
Add to students group
- Groups:
YN
- Change Summary:
-
Updated code (Cookie support, thumbnail shrink/expand), added screen shots
- Description:
-
Add Bookmarking feature in Diff viewer so users can
create and quickly jump between bookmarkers. Should help users moving back and forth between different parts of diffs when doing code reviews. 9/25 Starter code for the feature, with ghost bookmarker
chasing cursor and displaying which line in which file it is looking at. 10/3 Ghost bookmarker will now shrink and expand in
renponse to cursor position. Clicking on the ghost bookmarker will add bookmark DOM to the page. 10/10 Added styling for the actual bookmark as well as the
thumbnail. The thumbnail bookmark will appear in the position relative to the bookmark's position within the entire page, and will jump to where the bookmark is when clicked on it. + + 10/18 The thumbnail will also shrink/expand when the
+ cursor approaches. Added Cookie support so the bookmarks + will persist over page refresh, so the basics of the + feature are all in. Still need to bugfix for refreshing + the view on window resize, revision change, etc. The + extra for using random colors so bookmarks are easy to + differentiate is also to be implemented. - 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:
-
Add Bookmarking feature in Diff viewer so users can
create and quickly jump between bookmarkers. Should help users moving back and forth between different parts of diffs when doing code reviews. 9/25 Starter code for the feature, with ghost bookmarker
chasing cursor and displaying which line in which file it is looking at. 10/3 Ghost bookmarker will now shrink and expand in
renponse to cursor position. Clicking on the ghost bookmarker will add bookmark DOM to the page. 10/10 Added styling for the actual bookmark as well as the
thumbnail. The thumbnail bookmark will appear in the position relative to the bookmark's position within the entire page, and will jump to where the bookmark is when clicked on it. 10/18 The thumbnail will also shrink/expand when the
cursor approaches. Added Cookie support so the bookmarks will persist over page refresh, so the basics of the feature are all in. Still need to bugfix for refreshing the view on window resize, revision change, etc. The extra for using random colors so bookmarks are easy to differentiate is also to be implemented. + + 10/23 Bugfixes with bookmark positions and preventing
+ invalid bookmarks. Supports repositioning thumbnails at + window resize. Added a feature where bookmarks will have + random colors assigned from pre-defined list of colors + so they are easy to differentiate. May need more testing + but otherwise should be functional. - Testing Done:
-
~ So far only tested on local devserver.
~ Test cases (so far only tested manually on local devserver):
+ + 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 line + + Adding bookmark
+ - on file header should only display the filename + - on a line in a file should display the filename and the line number + - on an empty line should display with good styling + - on a line that wraps on browser as multiple lines should display with good styling + - added bookmark will appear again after refreshing on the page + - bookmark on an expanded chunk should appear again after collapsing the chunk and expanding again + - should have different colors for first 10 bookmarks + + Removing bookmark
+ - from a diff successfully removes bookmark and thumbnail + - from a new file successfully removes bookmark and thumbnail + - removed bookmark is still gone after refreshing on the page + + Bookmark thumbnail
+ - should appear where the main bookmark is relative to the whole page + - should have same color as the main bookmark + - expands when the cursor approaches + - shrinks when the cursor moves away + - jumps to where the main bookmark is when clicked + - appears in front of ghost bookmark + - is repositioned when window is resized + - is repositioned when a chunk is expanded/collapsed + - is repositioned when a file is loaded - 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:
-
Add Bookmarking feature in Diff viewer so users can
create and quickly jump between bookmarkers. Should help users moving back and forth between different parts of diffs when doing code reviews. 9/25 Starter code for the feature, with ghost bookmarker
chasing cursor and displaying which line in which file it is looking at. 10/3 Ghost bookmarker will now shrink and expand in
renponse to cursor position. Clicking on the ghost bookmarker will add bookmark DOM to the page. 10/10 Added styling for the actual bookmark as well as the
thumbnail. The thumbnail bookmark will appear in the position relative to the bookmark's position within the entire page, and will jump to where the bookmark is when clicked on it. 10/18 The thumbnail will also shrink/expand when the
cursor approaches. Added Cookie support so the bookmarks will persist over page refresh, so the basics of the feature are all in. Still need to bugfix for refreshing the view on window resize, revision change, etc. The extra for using random colors so bookmarks are easy to differentiate is also to be implemented. 10/23 Bugfixes with bookmark positions and preventing
invalid bookmarks. Supports repositioning thumbnails at window resize. Added a feature where bookmarks will have random colors assigned from pre-defined list of colors so they are easy to differentiate. May need more testing but otherwise should be functional. + + 10/31 Usability improvements. Changed cookie expiry to
+ one week and ghost bookmark color is slightly lighter. + Unified the representation of a bookmark to a single + object (previously called thumbnail), but tweaking directly + from previous implementation seem to cause few glitches, + therefore refactoring is needed for bugfix and polish. + The bookmark when created will animate, but at a folded + state; ideally we want the bookmark expanded at first + and then fold as it slides, which is still to be + implemented. - 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.
-
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.
-
-
-
-
I'm not sure if the cookie data should be passed in via
optionsor not. You should maybe ping Christian and ask him. -
-
-
-
-
-
-
-
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).
-
-
-
-
-
-
-
-
-
-
-
This should use sentence casing, e.g., begin with a capital and end with a period.
"from a clone of the"
-
-
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. -
Backbone.Model.getonly takes one key at a time, so this should be:e.get('fileDiffID'), e.get('interFileDiffID') -
-
-
-
-
-
-
-
-
-
-
-
-
-
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.
-
-
-
-
-
-
-
-
-
-
-
We can write this more clearly as :
return (filediff.id === fileDiffID && ((interFileDiff === null && interFileDiffID === null) || interFileDiff.id === interFileDiffID));This should use
===not==. -
-
-
-
YN
- Change Summary:
-
Animation at bookmark creation, moving cookie management to UserSession, code styles.
- Description:
-
Add Bookmarking feature in Diff viewer so users can
create and quickly jump between bookmarkers. Should help users moving back and forth between different parts of diffs when doing code reviews. 9/25 Starter code for the feature, with ghost bookmarker
chasing cursor and displaying which line in which file it is looking at. 10/3 Ghost bookmarker will now shrink and expand in
renponse to cursor position. Clicking on the ghost bookmarker will add bookmark DOM to the page. 10/10 Added styling for the actual bookmark as well as the
thumbnail. The thumbnail bookmark will appear in the position relative to the bookmark's position within the entire page, and will jump to where the bookmark is when clicked on it. 10/18 The thumbnail will also shrink/expand when the
cursor approaches. Added Cookie support so the bookmarks will persist over page refresh, so the basics of the feature are all in. Still need to bugfix for refreshing the view on window resize, revision change, etc. The extra for using random colors so bookmarks are easy to differentiate is also to be implemented. 10/23 Bugfixes with bookmark positions and preventing
invalid bookmarks. Supports repositioning thumbnails at window resize. Added a feature where bookmarks will have random colors assigned from pre-defined list of colors so they are easy to differentiate. May need more testing but otherwise should be functional. 10/31 Usability improvements. Changed cookie expiry to
one week and ghost bookmark color is slightly lighter. Unified the representation of a bookmark to a single object (previously called thumbnail), but tweaking directly from previous implementation seem to cause few glitches, therefore refactoring is needed for bugfix and polish. The bookmark when created will animate, but at a folded state; ideally we want the bookmark expanded at first and then fold as it slides, which is still to be implemented. + + 11/7 Animation when a new bookmark is created.
+ Code style fixes, including renaming 'bookmark' to + 'diffBookmark', moving cookie management to UserSession, + and adding doc comments. Few style issues, such as loose + equality checks, and confusing codes are yet to be + addressed. - 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:
-
Add Bookmarking feature in Diff viewer so users can
create and quickly jump between bookmarkers. Should help users moving back and forth between different parts of diffs when doing code reviews. 9/25 Starter code for the feature, with ghost bookmarker
chasing cursor and displaying which line in which file it is looking at. 10/3 Ghost bookmarker will now shrink and expand in
renponse to cursor position. Clicking on the ghost bookmarker will add bookmark DOM to the page. 10/10 Added styling for the actual bookmark as well as the
thumbnail. The thumbnail bookmark will appear in the position relative to the bookmark's position within the entire page, and will jump to where the bookmark is when clicked on it. 10/18 The thumbnail will also shrink/expand when the
cursor approaches. Added Cookie support so the bookmarks will persist over page refresh, so the basics of the feature are all in. Still need to bugfix for refreshing the view on window resize, revision change, etc. The extra for using random colors so bookmarks are easy to differentiate is also to be implemented. 10/23 Bugfixes with bookmark positions and preventing
invalid bookmarks. Supports repositioning thumbnails at window resize. Added a feature where bookmarks will have random colors assigned from pre-defined list of colors so they are easy to differentiate. May need more testing but otherwise should be functional. 10/31 Usability improvements. Changed cookie expiry to
one week and ghost bookmark color is slightly lighter. Unified the representation of a bookmark to a single object (previously called thumbnail), but tweaking directly from previous implementation seem to cause few glitches, therefore refactoring is needed for bugfix and polish. The bookmark when created will animate, but at a folded state; ideally we want the bookmark expanded at first and then fold as it slides, which is still to be implemented. 11/7 Animation when a new bookmark is created.
Code style fixes, including renaming 'bookmark' to 'diffBookmark', moving cookie management to UserSession, and adding doc comments. Few style issues, such as loose equality checks, and confusing codes are yet to be addressed. + + 11/15 Rewriting cookie management so cookies are saved
+ per fragment for more space as well as compatibility + with not just revision switch but also with pagination. + More code style cleanup and fixing confusing codes. + Needs to be tested again since many parts have been + rewritten, and automated tests are still on queue. - 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:
-
Add Bookmarking feature in Diff viewer so users can
create and quickly jump between bookmarkers. Should help users moving back and forth between different parts of diffs when doing code reviews. 9/25 Starter code for the feature, with ghost bookmarker
chasing cursor and displaying which line in which file it is looking at. 10/3 Ghost bookmarker will now shrink and expand in
renponse to cursor position. Clicking on the ghost bookmarker will add bookmark DOM to the page. 10/10 Added styling for the actual bookmark as well as the
thumbnail. The thumbnail bookmark will appear in the position relative to the bookmark's position within the entire page, and will jump to where the bookmark is when clicked on it. 10/18 The thumbnail will also shrink/expand when the
cursor approaches. Added Cookie support so the bookmarks will persist over page refresh, so the basics of the feature are all in. Still need to bugfix for refreshing the view on window resize, revision change, etc. The extra for using random colors so bookmarks are easy to differentiate is also to be implemented. 10/23 Bugfixes with bookmark positions and preventing
invalid bookmarks. Supports repositioning thumbnails at window resize. Added a feature where bookmarks will have random colors assigned from pre-defined list of colors so they are easy to differentiate. May need more testing but otherwise should be functional. 10/31 Usability improvements. Changed cookie expiry to
one week and ghost bookmark color is slightly lighter. Unified the representation of a bookmark to a single object (previously called thumbnail), but tweaking directly from previous implementation seem to cause few glitches, therefore refactoring is needed for bugfix and polish. The bookmark when created will animate, but at a folded state; ideally we want the bookmark expanded at first and then fold as it slides, which is still to be implemented. 11/7 Animation when a new bookmark is created.
Code style fixes, including renaming 'bookmark' to 'diffBookmark', moving cookie management to UserSession, and adding doc comments. Few style issues, such as loose equality checks, and confusing codes are yet to be addressed. 11/15 Rewriting cookie management so cookies are saved
per fragment for more space as well as compatibility with not just revision switch but also with pagination. More code style cleanup and fixing confusing codes. Needs to be tested again since many parts have been rewritten, and automated tests are still on queue. + + 11/21 Fixed a bug with bookmarks not being saved when
+ on hashed pages, and few more coding style cleanups. + Added some automated tests for DiffBookmarkCollection + and for DiffBookmarksContainerView. + More tests to be added. - 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:
-
(WIP) Add Bookmarking feature to Diff Viewer.Add Bookmarking feature to Diff Viewer.
- Description:
-
~ Add Bookmarking feature in Diff viewer so users can
~ create and quickly jump between bookmarkers. Should help ~ users moving back and forth between different parts of ~ 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. - diffs when doing code reviews. ~ 9/25 Starter code for the feature, with ghost bookmarker
~ chasing cursor and displaying which line in which file ~ it is looking at. ~ 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. ~ 10/3 Ghost bookmarker will now shrink and expand in
~ renponse to cursor position. Clicking on the ~ ghost bookmarker will add bookmark DOM to the page. ~ ~ 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. - 10/10 Added styling for the actual bookmark as well as the
- thumbnail. The thumbnail bookmark will appear in the position - relative to the bookmark's position within the entire page, - and will jump to where the bookmark is when clicked on it. - - 10/18 The thumbnail will also shrink/expand when the
- cursor approaches. Added Cookie support so the bookmarks - will persist over page refresh, so the basics of the - feature are all in. Still need to bugfix for refreshing - the view on window resize, revision change, etc. The - extra for using random colors so bookmarks are easy to - differentiate is also to be implemented. - - 10/23 Bugfixes with bookmark positions and preventing
- invalid bookmarks. Supports repositioning thumbnails at - window resize. Added a feature where bookmarks will have - random colors assigned from pre-defined list of colors - so they are easy to differentiate. May need more testing - but otherwise should be functional. - - 10/31 Usability improvements. Changed cookie expiry to
- one week and ghost bookmark color is slightly lighter. - Unified the representation of a bookmark to a single - object (previously called thumbnail), but tweaking directly - from previous implementation seem to cause few glitches, - therefore refactoring is needed for bugfix and polish. - The bookmark when created will animate, but at a folded - state; ideally we want the bookmark expanded at first - and then fold as it slides, which is still to be - implemented. - - 11/7 Animation when a new bookmark is created.
- Code style fixes, including renaming 'bookmark' to - 'diffBookmark', moving cookie management to UserSession, - and adding doc comments. Few style issues, such as loose - equality checks, and confusing codes are yet to be - addressed. - - 11/15 Rewriting cookie management so cookies are saved
- per fragment for more space as well as compatibility - with not just revision switch but also with pagination. - More code style cleanup and fixing confusing codes. - Needs to be tested again since many parts have been - rewritten, and automated tests are still on queue. - - 11/21 Fixed a bug with bookmarks not being saved when
- on hashed pages, and few more coding style cleanups. - Added some automated tests for DiffBookmarkCollection - and for DiffBookmarksContainerView. - More tests to be added. - Testing Done:
-
~ Test cases (so far only tested manually on local devserver):
~ 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 line ~ Adding bookmark
~ Bookmarks
+ - 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 empty line should display with good styling ~ - on a line that wraps on browser as multiple lines should display with good styling ~ - added bookmark will appear again after refreshing on the page ~ - bookmark on an expanded chunk should appear again after collapsing the chunk and expanding again ~ - should have different colors for first 10 bookmarks ~ - 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 removal ~ Removing bookmark
~ - - from a diff successfully removes bookmark and thumbnail - - from a new file successfully removes bookmark and thumbnail - - removed bookmark is still gone after refreshing on the page ~ Bookmark thumbnail
~ - should appear where the main bookmark is relative to the whole page ~ - should have same color as the main bookmark ~ - expands when the cursor approaches ~ - shrinks when the cursor moves away ~ - jumps to where the main bookmark is when clicked ~ - appears in front of ghost bookmark ~ - is repositioned when window is resized ~ - is repositioned when a chunk is expanded/collapsed ~ - is repositioned when a file is loaded ~ Following 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 - 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
- Diff:
-
Revision 11 (+1317 -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 -
-
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. -
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. -
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. -
-
We namespace these names, so these would be best as
@bookmark-ghost-bg-colorand@bookmark-default-bg-color, in alphabetical order. -
-
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?
-
-
-
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.) -
-
-
-
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.
-
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.
-
Multi-line comments must always be in this form:
/* * line * line */Same with other comments below.
-
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.
-
-
-
-
-
-
-
We only use one
varstatement per function, with one variable per line, like:var collection, listenerObj; -
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. -
-
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.
-
-
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.
-
-
When named
template, it implies it's the template for the view itself. You probably want to call thisghostBookmarkTemplate. -
-
-
-
Any member variables that don't need public access should be made private (prefix the name with a
_). -
-
rendershouldn't be responsible for deciding where its main element is put. That's the responsibility of the caller, only. -
-
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. -
Can you tell me what the
bodyquery is about? Is this about whether it's currently packed into the page somewhere? -
-
The list of variables should begin with defined variables, so:
var cookie = JSON.parse(cookieString), bookmark; -
-
-
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(). -
-
.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. -
-
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? -
-
-
-
.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. -
-
-
-
-
-
-
-
-
-
-
-
This will cause problems. You need to instead do:
this.$el.html(this.template());Also, this must happen in
render(). -
-
-
-
-
-
Blank line between these.
vars andreturns generally should be thought of as their own "paragraphs" of code. -
-
-
-
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
{. -
-
-
-
-
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