Fix commenting on text-based file attachments.
Review Request #7092 — Created March 19, 2015 and submitted
The recent work for retrofitting text-based file attachments for diff
support triggered some bad behavior in the commenting support.
TextCommentRowSelectorand comment flag placement needed to work on a
table rather than a tbody. This caused mismatches between the element's
list of rows and the rowIndexes. It also caused a reference to an
invalid variable for comment placement.One possible solution is to update
TextCommentRowSelectorto work with
tbodies, and a previous attempt went this route. However, future work
for diffviewer-style diffs for text attachments will require multiple
tbodies per table, so instead, this change splits the tbodies back out
into their own tables again.The tables, for both plain text and rendered text, are split off into
reusable templates, reducing the amount of code duplication. The
revision information has been moved into these tables, under the tabs.
This will help with future review UIs that want to render plain text,
but also want a different UI entirely under the Rendered tab (such as
SVGs).The revision label and selector rows are now hard-coded in the template,
as well, rather than being dynamically added later.There's also some visual fixes for the page to prevent unwanted
gradients and to fix borders at the bottom of the view.
Tested reviewing individual revisions and diffs of plain text and Markdown
attachments, both source and rendered.Verified in Chrome and Firefox that the visual styles were correct.
Change Summary:
Switched to
yesnoin another location.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+42 -29) |
-
Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/static/rb/js/views/textCommentRowSelector.js Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/static/rb/js/views/textCommentRowSelector.js
WU
-
-
reviewboard/static/rb/js/views/textBasedReviewableView.js (Diff revision 2) tBodiescannot be used here -
reviewboard/static/rb/js/views/textBasedReviewableView.js (Diff revision 2) Same as above,
tBodiesneed to be removed.
Change Summary:
Fixed scrolling to lines by removing the
tBodiesreferences.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+44 -31) |
-
Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/static/rb/js/views/textCommentRowSelector.js Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/static/rb/js/views/textCommentRowSelector.js
-
I'm actually going to need to redo this change. I need to go back to actually using tables, to prepare for actual diffing (since diffs need tbodies in a dedicated table).
Change Summary:
Redid the change to instead move the tbodies back into their own tables.
This will be needed in order to support a real diff view for the text files,
since diffs need to insert multiple tbodies.Visually, the only difference is that revision labels now appear below the
tabs instead of above, for renderable text files. This is actually going to
let us do a better job of controlling the view later for fancier text review
UIs, such as for SVGs, where you want to display a text mode, but also render
with a different UI entirely.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+153 -146) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
-
Tool: PEP8 Style Checker Ignored Files: reviewboard/templates/reviews/ui/_text_rendered_table.html reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/templates/reviews/ui/text.html reviewboard/templates/reviews/ui/_text_table.html Tool: Pyflakes Ignored Files: reviewboard/templates/reviews/ui/_text_rendered_table.html reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/templates/reviews/ui/text.html reviewboard/templates/reviews/ui/_text_table.html
-
-
reviewboard/static/rb/js/views/textBasedReviewableView.js (Diff revision 3) This could be:
lineNum = RB.MathUtils.clip(lineNum, 1, rows.length) - 1;
Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/static/rb/js/views/textCommentRowSelector.js Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/static/rb/js/views/textCommentRowSelector.js