Better Change Description rendering
Review Request #2966 — Created March 17, 2012 and discarded
NOTE: this is currently work in progress. I am trying to see if I can modify the style to be more GitHub like Right now, when a user changes various fields on the review request (only summary and description atm), we show a box that has both the new and old text. It will show what changed in a more usable manner.
Tested in my local linux machine with Chrome using the following steps 1. take a review request, change its description/testing done field 2. publish the change 3. see that the rendering for changes in description/testing done under "Review request changed" have been rendered in an interdiff style accordingly
Screenshots
| Description | From | Last Updated |
|---|---|---|
|
Two blank lines between top-level methods. There's way too much duplication of this function with the other. Try to split … |
 chipx86
|
|
|
There should be a one-line summary of this method on the first line right after '''. |
AM ammok | |
|
chuck => chunks. missing a period. |
AM ammok | |
|
There's enough room to fit this on one line. |
chipx86
|
|
|
Must be in the list alphabetically. |
chipx86
|
|
|
Two blank lines between top-level functions. |
chipx86
|
|
|
There should be two lines here. |
AM ammok | |
|
I'd move this into the changedescs/ app (it's really all about the Change Descriptions, afterall) and name this something else. … |
chipx86
|
|
|
Why can't this be part of the code that generates these? |
chipx86
|
|
|
No space before the ":" |
chipx86
|
|
|
One-space indentation in the HTML files. |
chipx86
|
|
|
No need for the trailing space after class declarations. i.e: |
ME medanat | |
|
the {% should not be indented. Just the contents of it. |
chipx86
|
|
|
No spaces after {{ and before }}. |
chipx86
|
|
|
From pep8, don't use "old[-1] != '\n':" use "not old.endswith('\n'):" |
ME medanat | |
|
Alphabetical order for loads. |
ME medanat | |
|
Place those in trans blocks. |
ME medanat | |
|
Single space indentation for html tags. Indentation for template tags should be within the {% %}. i.e.: don't indent the … |
ME medanat | |
|
Loads should be in alphabetical order. |
ME medanat |
JI
Change Summary:
Final change before I came up with a new style of rendering
Diff: |
Revision 4 (+135 -6) |
||
|---|---|---|---|
Screenshots: |
|
JI
Change Summary:
Update the descriptions
Description: |
|
|---|
-
-
reviewboard/diffviewer/diffutils.py (Diff revision 5) Two blank lines between top-level methods. There's way too much duplication of this function with the other. Try to split the common logic between them out into reusable functions. diff_line could still be reused, for example. Sure, you won't need the meta information, so that'll just be turned into an optional parameter to the function, but that consolidates a bunch right htere. Same with new_chunk. Just make these things optional. There are no interesting lines or metadata to go over, so just ensure they become no-ops. Really, though, anything remaining for ChangeDescription rendering probably belongs in the changedescs/ app, and not here.
-
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 5) Must be in the list alphabetically.
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 5) Two blank lines between top-level functions.
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 5) I'd move this into the changedescs/ app (it's really all about the Change Descriptions, afterall) and name this something else. Maybe changedesc_field_diff.
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 5) Why can't this be part of the code that generates these?
-
-
reviewboard/templates/reviews/diff_multiline_fragment.html (Diff revision 5) One-space indentation in the HTML files.
-
reviewboard/templates/reviews/diff_multiline_fragment.html (Diff revision 5) the {% should not be indented. Just the contents of it. -
reviewboard/templates/reviews/diff_multiline_fragment.html (Diff revision 5) No spaces after {{ and before }}.
AM
-
-
reviewboard/diffviewer/diffutils.py (Diff revision 5) There should be a one-line summary of this method on the first line right after '''.
-
-
reviewboard/diffviewer/diffutils.py (Diff revision 5) Could (a, markup_a) and (b, markup_b) be returned as tuples by some helper function to reduce duplication? Or would it be ill-advised to return tuples like that? (old and new are being modified, and not used later, but it might make sense to keep those changes around so no surprises are introduced by later modifications)
-
ME
-
Not sure what's been fixed so far, but I have one more minor style issue.
-
reviewboard/templates/reviews/diff_multiline_fragment.html (Diff revision 5) No need for the trailing space after class declarations. i.e: <col class="line"/>
ME
-
A few issues, some that might have been brought up before.
-
reviewboard/diffviewer/diffutils.py (Diff revision 7) From pep8, don't use "old[-1] != '\n':" use "not old.endswith('\n'):" -
reviewboard/templates/reviews/diff_changedesc_field.html (Diff revision 7) Alphabetical order for loads.
-
reviewboard/templates/reviews/diff_changedesc_field.html (Diff revision 7) Place those in trans blocks.
-
reviewboard/templates/reviews/diff_changedesc_field.html (Diff revision 7) Single space indentation for html tags. Indentation for template tags should be within the {% %}. i.e.: don't indent the "{%", indent within the "{%" "%}". -
reviewboard/templates/reviews/review_detail.html (Diff revision 7) Loads should be in alphabetical order.