Summary

Rendering pluggable UIs in lightbox

Review Request #3526 — Created Nov. 17, 2012 and discarded Nov. 19, 2013, 4:20 p.m.

Information

Owner

aam1r

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Review UIs can now be shown inline in a lightbox (i.e. modalBox for now) window rather than having the user render a new page every time. 

To display a review UI as inline, "allow_inline" needs to be set to True in the Review UI subclass (see markdownui.py in the diff for an example). Custom URL patterns can also be defined by a Review UI subclass that can be used in the inline pluggable UI to allow additional custom functionality (such as fetching data through the server using AJAX).

Testing Done

Inline enabled for Markdown Review UI:
- Uploaded a Markdown file with no caption. Clicked "Review". Modal box popped up with filename as title. Displayed the rendered contents of the attachment.
- Uploaded a Markdown file with a caption. Clicked "Review". Modal box popped up with the attachment caption as title. Displayed the rendered contents of the attachment.
- Uploaded a Markdown file with no content. Clicked "Review". Modal box popped up. Displayed empty modal box (since attachment was originally empty).

Inline disabled for Markdown Review UI:
- Uploaded a Markdown file. Clicked "Review". Got redirected to the review UI page; no modal box was shown.

Custom URL pattern initialized in Markdown Review UI:
- Set a custom URL pattern in Markdown Review UI class. Accessing the URL manually (using curl and my browser) loaded the view of that URL pattern, as expected.

Files

Issues

Description	From	Last Updated
Probably should be the attachment's display name, or filename if that's not there.	chipx86	Nov. 22, 2012, 5:06 a.m.
I probably screwed up, but there should be two blank lines.	chipx86	Nov. 19, 2012, 5:59 p.m.
continuation line over-indented for hanging indent	JS jsintal	Nov. 20, 2012, 5:28 p.m.
Since this is a function, it should be prefixed with get_() I think actually, just get_urlpatterns, since the context should …	chipx86	Nov. 19, 2012, 5:59 p.m.
When comparing against not, use "is" or "is not." In this case, though, don't do that comparison, just "if not …	chipx86	Nov. 19, 2012, 5:59 p.m.
We already have this in mimetypes.py, so let's not duplicate it here.	chipx86	Nov. 20, 2012, 4:39 p.m.
Separate these. Anything imported from reviewboard.* is in the "current project imports" group. "django" is in the "3rd party imports" …	chipx86	Nov. 20, 2012, 4:41 p.m.
URLs should always end with "/". "fetch" is a weird URL. It's not likely what we want. Maybe "rendered."	chipx86	Nov. 19, 2012, 5:59 p.m.
line too long	JS jsintal	Nov. 20, 2012, 5:28 p.m.
line too long	JS jsintal	Nov. 21, 2012, 7:39 a.m.
Should be aligned with "urlpatterns"	chipx86	Nov. 19, 2012, 5:59 p.m.
Should be two blank lines.	chipx86	Nov. 19, 2012, 5:59 p.m.
Alignment looks off, but no need to have one per line. Do you mean for this function to still be …	chipx86	Nov. 25, 2012, 12:38 p.m.
Where did the original files go? Merging issue?	chipx86	Nov. 19, 2012, 6:05 p.m.
This is global. This should all be handled by the ReviewUI subclass. The click handler should only call something on …	chipx86	Nov. 25, 2012, 12:38 p.m.
The title should include the attachment display name or filename.	chipx86	Nov. 19, 2012, 6:26 p.m.
Why are you limiting this to 500px?	david	Nov. 24, 2012, 5:07 a.m.
We just hit some problems with dynamic URL handling for extensions, and came up with a nifty thing called DynamicURLResolver. …	chipx86	Nov. 30, 2012, 4:32 a.m.
Blank line between these. Third-party imports and current project imports must be in their own import groups.	chipx86	Nov. 29, 2012, 7:53 p.m.
It'd be better to do ".... import get_urlpatterns as get_review_ui_urlpatterns", to make it clear what it is we're accessing.	chipx86	Nov. 29, 2012, 7:42 p.m.
I was confused at first about which 'this' we were using. Maybe instead of passing the link, pass the URL.	chipx86	Nov. 29, 2012, 7:53 p.m.
Use 1-space indentation, and make sure parameters align.	chipx86	Nov. 30, 2012, 4:48 a.m.
Since mimeparse is 3rd party, it should be with the 2nd group of imports, right under: from djblets.util.urlresolvers import DynamicURLResolver	SL slchen	Dec. 4, 2012, 5:30 p.m.
Exceeds max line length: from reviewboard.reviews.ui.base import get_urlpatterns \ as get_review_ui_urlpatterns	SL slchen	Dec. 5, 2012, 4:16 a.m.

reviewboard/reviews/ui/base.py (Diff revision 1)

Is there a more Pythonic way of doing this?

Change Summary:

Added client-side code to perform AJAX request when "Review" is clicked for a file attachment and displayed in a modal box.

Description:

		The goal of this review request is to render pluggable UIs in a lightbox window (rather than always opening them in a new window).

		Done so far:
		- Added support for each Review UI to specify "urlpatterns". These patterns are included in reviews/urls.py.
~		- Implemented "fetch_rendered_attachment" which renders the HTML of the file attachment and renders it as plain-text rather than HTML so it can be used with AJAX queries.
	~	- Implemented "fetch_rendered_attachment" which renders the HTML of the file attachment and renders it as plain-text rather than HTML.
	+	- When "Review" is clicked for a file attachment, an AJAX request is performed which fetches the rendered HTML and displays it in a modal box.

		Note: This code has been branched off the "markdown-ui" code (r/3434)

Diff:

Revision 2 (+157 -7)

Show changes

	setup.py
	reviewboard/settings.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/reviews/ui/__init__.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/ui/markdownui.py
	reviewboard/static/rb/js/reviews.js
	3 more

Change Summary:

Added Testing information

Testing Done:

	+	Used curl and manually checked append "/fetch" at the end of "/r/{review_request_id}/file/{file_attachment_id}" to make sure that rendered HTML is returned
	+	Tested the "Review" click action handling by clicking the link in my browser
	+
	+	The above tests were only done for valid Markdown file attachments.

Change Summary:

Added screenshot of modal box

Screenshots:

+	Rendered Markdown in a modal box

Rendered Markdown in a modal box

The issue has been resolved. Show all issues

Probably should be the attachment's display name, or filename if that's not there.

reviewboard/reviews/ui/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
I probably screwed up, but there should be two blank lines.
```

reviewboard/reviews/ui/base.py (Diff revision 2)

The issue has been resolved. Show all issues

Since this is a function, it should be prefixed with get_()

I think actually, just get_urlpatterns, since the context should be clear from the file. Importers can alias it if needed.

reviewboard/reviews/ui/base.py (Diff revision 2)

The issue has been resolved. Show all issues

When comparing against not, use "is" or "is not." In this case, though, don't do that comparison, just "if not review_ui.urlpatterns".

That way, urlpatterns can be [] and it'll do the correct thing.

reviewboard/reviews/ui/base.py (Diff revision 2)
The issue has been resolved. Show all issues
```
We already have this in mimetypes.py, so let's not duplicate it here.
```

reviewboard/reviews/ui/markdownui.py (Diff revision 2)

The issue has been resolved. Show all issues

Separate these.

Anything imported from reviewboard.* is in the "current project imports" group. "django" is in the "3rd party imports" group.

reviewboard/reviews/ui/markdownui.py (Diff revision 2)

The issue has been resolved. Show all issues

URLs should always end with "/".

"fetch" is a weird URL. It's not likely what we want. Maybe "rendered."

reviewboard/reviews/ui/markdownui.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should be aligned with "urlpatterns"
```
reviewboard/reviews/urls.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should be two blank lines.
```

reviewboard/reviews/views.py (Diff revision 2)

The issue has been resolved. Show all issues

Alignment looks off, but no need to have one per line.

Do you mean for this function to still be here, though? This should only exist in the given ReviewUIs that need to do such things, and should not be common.

aam1r Nov. 23, 2012, 1:57 p.m.

I tried moving this function to the MarkdownReviewUI class and made it a class method. I also changed the URL pattern to be:

(r'^(?P<review_request_id>[0-9]+)/file/(?P<file_attachment_id>[0-9]+)/rendered/$', 'reviewboard.reviews.ui.markdownui.rendered_attachment')

but that didn't work. I get the following error: "Could not import reviewboard.reviews.ui.markdownui.rendered_attachment. View does not exist in module reviewboard.reviews.ui.markdownui." (http://i.imgur.com/2j9CL.png)

Moving the function outside of that class works though but I think it should ideally be part of the ReviewUI subclass. Any ideas on how I can make a classmethod work?

reviewboard/settings.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Where did the original files go? Merging issue?
```

reviewboard/static/rb/js/reviews.js (Diff revision 2)

The issue has been resolved. Show all issues

This is global. This should all be handled by the ReviewUI subclass. The click handler should only call something on ReviewUI.

Remember not not all ReviewUIs need to fetch data to display on the server.

The only part that should be common is the modalBox.

aam1r Nov. 23, 2012, 1:57 p.m.

Moved this to the "showReviewUI()" function defined in the same file but that still needs some work to be done. Instead of hardcoding it to be "rendered", I need to make it so that it first checks whether the ReviewUI does support lightbox displaying. Once that has been verified, it needs to get the URL for to get the HTML data to render.

reviewboard/templates/reviews/ui/markdown.html (Diff revision 2)
The issue has been resolved. Show all issues
```
The title should include the attachment display name or filename.
```

reviewboard/reviews/ui/base.py (Diff revision 2)
The issue has been dropped. Show all issues
```
continuation line over-indented for hanging indent
```
reviewboard/reviews/ui/markdownui.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long
```

reviewboard/reviews/ui/markdownui.py (Diff revision 2)

The issue has been dropped. Show all issues

line too long

aam1r Nov. 20, 2012, 5:28 p.m.

Not sure how I can make it any shorter. Several lines in reviews/urls.py also have the same issue.

Any ideas?

david Nov. 20, 2012, 5:32 p.m.

Don't worry about the line length in urls.py. There's no good way to wrap the regexps.

Change Summary:

Addressed most issues

Description:

		The goal of this review request is to render pluggable UIs in a lightbox window (rather than always opening them in a new window).

		Done so far:
		- Added support for each Review UI to specify "urlpatterns". These patterns are included in reviews/urls.py.
~		- Implemented "fetch_rendered_attachment" which renders the HTML of the file attachment and renders it as plain-text rather than HTML.
	~	- Implemented "rendered_attachment" which renders the HTML of the file attachment and renders it as plain-text rather than HTML.
		- When "Review" is clicked for a file attachment, an AJAX request is performed which fetches the rendered HTML and displays it in a modal box.

		Note: This code has been branched off the "markdown-ui" code (r/3434)

Testing Done:

~		Used curl and manually checked append "/fetch" at the end of "/r/{review_request_id}/file/{file_attachment_id}" to make sure that rendered HTML is returned
	~	Used curl and manually checked append "/rendered/" at the end of "/r/{review_request_id}/file/{file_attachment_id}" to make sure that rendered HTML is returned
		Tested the "Review" click action handling by clicking the link in my browser

		The above tests were only done for valid Markdown file attachments.

Diff:

Revision 3 (+973 -165)

Show changes

	README
	docs/manual/docs.db
	docs/manual/admin/installation/linux.txt
	docs/manual/users/tools/post-review.txt
	reviewboard/accounts/backends.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/reviews/admin.py
	reviewboard/reviews/datagrids.py
	31 more

Screenshots:

+	Caption shown as title (when available)
+	Filename shown as title as a fallback

Change Summary:

Fixed git issues

Diff:

Revision 4 (+74 -8)

Show changes

	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/ui/markdownui.py
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html

reviewboard/reviews/ui/base.py (Diff revision 4)

If you wanted to make this cuter, you could use a list comprehension.

aam1r Nov. 24, 2012, 5:07 a.m.

How would that list comprehension look?

I am trying: 
    urls = [ui.urlpatterns for ui in _file_attachment_review_uis if ui.urlpatterns]

But that gives a different result than using a loop:

(Pdb) urlpatterns
[<RegexURLPattern None ^(?P<review_request_id>[0-9]+)/file/(?P<file_attachment_id>[0-9]+)/rendered/$>]
(Pdb) urls
[[<RegexURLPattern None ^(?P<review_request_id>[0-9]+)/file/(?P<file_attachment_id>[0-9]+)/rendered/$>]]

david Nov. 24, 2012, 4:57 p.m.

It's making a list of lists, so you can flatten it like this:
itertools.chain(*urls)

That said, maybe it's better to be clearer than cuter :)

reviewboard/static/rb/js/reviews.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Why are you limiting this to 500px?
```

Change Summary:

Addressed a few issues and removed 'urlpatterns' stuff from MarkdownReviewUI since it does not need it.

Diff:

Revision 5 (+50 -8)

Show changes

	reviewboard/reviews/urls.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/ui/markdownui.py
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html

Change Summary:

Updated "Description" and "Testing Done"

Description:

		The goal of this review request is to render pluggable UIs in a lightbox window (rather than always opening them in a new window).

		Done so far:
		- Added support for each Review UI to specify "urlpatterns". These patterns are included in reviews/urls.py.
~		- Implemented "rendered_attachment" which renders the HTML of the file attachment and renders it as plain-text rather than HTML.
~		- When "Review" is clicked for a file attachment, an AJAX request is performed which fetches the rendered HTML and displays it in a modal box.
	~	- When "Review" is clicked for a file attachment, an AJAX request is performed to "?inline=1" which fetches the rendered HTML and displays it in a modal box.
	~	- A file caption is now shown in the modal box

		Note: This code has been branched off the "markdown-ui" code (r/3434)

Testing Done:

~		Used curl and manually checked append "/rendered/" at the end of "/r/{review_request_id}/file/{file_attachment_id}" to make sure that rendered HTML is returned
~		Tested the "Review" click action handling by clicking the link in my browser
	~	Added a urlpattern for a ReviewUI and tested it by manually visiting the URL specified in the pattern
	~	Tested the "Review" click action handling by manually clicking the "Review" link in my browser
-
-		The above tests were only done for valid Markdown file attachments.

Change Summary:

Removed outdated screenshot

Screenshots:

-	Rendered Markdown in a modal box

Change Summary:

Updated description and testing done. Review request no longer in WIP.

Summary:

-	WIP - Rendering pluggable UIs in lightbox
+	Rendering pluggable UIs in lightbox

Description:

~		The goal of this review request is to render pluggable UIs in a lightbox window (rather than always opening them in a new window).
	~	Review UIs can now be shown inline in a lightbox (i.e. modalBox for now) window rather than having the user render a new page every time.

~		Done so far:
	~	To display a review UI as inline, "allow_inline" needs to be set to True in the Review UI subclass (see markdownui.py in the diff for an example). Custom URL patterns can also be defined by a Review UI subclass that can be used in the inline pluggable UI to allow additional custom functionality (such as fetching data through the server using AJAX).
-		- Added support for each Review UI to specify "urlpatterns". These patterns are included in reviews/urls.py.
-		- When "Review" is clicked for a file attachment, an AJAX request is performed to "?inline=1" which fetches the rendered HTML and displays it in a modal box.
-		- A file caption is now shown in the modal box

~		Note: This code has been branched off the "markdown-ui" code (r/3434)
	~	Note: The parent of this review request is r/3434. That review request should be merged in before merging this.

Testing Done:

~		Added a urlpattern for a ReviewUI and tested it by manually visiting the URL specified in the pattern
~		Tested the "Review" click action handling by manually clicking the "Review" link in my browser
	~	Inline enabled for Markdown Review UI:
	~	- Uploaded a Markdown file with no caption. Clicked "Review". Modal box popped up with filename as title. Displayed the rendered contents of the attachment.
	+	- Uploaded a Markdown file with a caption. Clicked "Review". Modal box popped up with the attachment caption as title. Displayed the rendered contents of the attachment.
	+	- Uploaded a Markdown file with no content. Clicked "Review". Modal box popped up. Displayed empty modal box (since attachment was originally empty).
	+
	+	Inline disabled for Markdown Review UI:
	+	- Uploaded a Markdown file. Clicked "Review". Got redirected to the review UI page; no modal box was shown.
	+
	+	Custom URL pattern initialized in Markdown Review UI:
	+	- Set a custom URL pattern in Markdown Review UI class. Accessing the URL manually (using curl and my browser) loaded the view of that URL pattern, as expected.

Diff:

Revision 6 (+52 -11)

Show changes

	reviewboard/reviews/urls.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/ui/markdownui.py
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/templates/reviews/ui/base_inline.html

Change Summary:

Removing screenshots to add new ones

Screenshots:

-	Caption shown as title (when available)
-	Filename shown as title as a fallback

Change Summary:

Added screenshots

Added Files:

Change Summary:

Stripped down base template to only include custom Javascript and HTML. Tested it by launching modal box several times for several attachments -- all seems to work.

Diff:

Revision 7 (+47 -13)

Show changes

	reviewboard/reviews/urls.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/ui/markdownui.py
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/templates/reviews/ui/base_inline.html

reviewboard/reviews/ui/base.py (Diff revision 7)

The issue has been resolved. Show all issues

We just hit some problems with dynamic URL handling for extensions, and came up with a nifty thing called DynamicURLResolver. We should use that here so that we can make it work when extensions are enabled/disabled.

You can import it from djblets.util.urlresolvers.

You should then do:

_dynamic_urls = DynamicURLResolver()

def get_urlpatterns():
    urlpatterns = patterns('', _dynamic_urls)

    for blah:
        _dynamic_urls.add_patterns(review_ui.urlpatterns)

return urlpatterns


Registration should then do an add_patterns on the reviewui's urlpatterns, and unregistration should do remove_paterns.

reviewboard/reviews/urls.py (Diff revision 7)

The issue has been resolved. Show all issues

Blank line between these. Third-party imports and current project imports must be in their own import groups.

reviewboard/reviews/urls.py (Diff revision 7)

The issue has been resolved. Show all issues

It'd be better to do ".... import get_urlpatterns as get_review_ui_urlpatterns", to make it clear what it is we're accessing.

reviewboard/static/rb/js/reviews.js (Diff revision 7)

The issue has been resolved. Show all issues

I was confused at first about which 'this' we were using. Maybe instead of passing the link, pass the URL.

reviewboard/templates/reviews/review_request_box.html (Diff revision 7)
The issue has been resolved. Show all issues
```
Use 1-space indentation, and make sure parameters align.
```

Change Summary:

Addressed issues

Diff:

Revision 8 (+53 -13)

Show changes

	setup.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/ui/__init__.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/ui/markdownui.py
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/js/pipeline.html
	reviewboard/templates/reviews/review_request_box.html
	1 more

Change Summary:

Removed accidentally committed file that wasn't part of this commit.

Diff:

Revision 9 (+52 -12)

Show changes

	setup.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/ui/__init__.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/ui/markdownui.py
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/templates/reviews/ui/base_inline.html

Change Summary:

This review request is no longer dependent on Markdown Review UI. It can be merged in before r/3434.

Description:

		Review UIs can now be shown inline in a lightbox (i.e. modalBox for now) window rather than having the user render a new page every time.

		To display a review UI as inline, "allow_inline" needs to be set to True in the Review UI subclass (see markdownui.py in the diff for an example). Custom URL patterns can also be defined by a Review UI subclass that can be used in the inline pluggable UI to allow additional custom functionality (such as fetching data through the server using AJAX).
-
-		Note: The parent of this review request is r/3434. That review request should be merged in before merging this.

Diff:

Revision 10 (+51 -11)

Show changes

	reviewboard/reviews/urls.py
	reviewboard/reviews/ui/base.py
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/templates/reviews/ui/base_inline.html

reviewboard/reviews/ui/base.py (Diff revision 10)

The issue has been resolved. Show all issues

Since mimeparse is 3rd party, it should be with the 2nd group of imports, right under:

from djblets.util.urlresolvers import DynamicURLResolver

aam1r Dec. 3, 2012, 8:12 a.m.

Hmm, but isn't djblets also considered 3rd party? 

I guess even then, alphabetically it makes sense to put "import mimeparse" after "from * import *". I will fix this. Thanks!

chipx86 Dec. 4, 2012, 12:44 a.m.

Yes, from reviewboard's perspective, it's third party.

Looks great Aamir! Looking forward to use this with the inline file attachments UI.

reviewboard/static/rb/js/reviews.js (Diff revision 10)

I'm wondering what will happen when there's unsaved data in the dlg when you try to close it. Will it trigger the check for unsaved actions when you destroy the dlg?

aam1r Dec. 3, 2012, 7:40 p.m.

Tina, could you emphasize a bit more? Not sure what unsaved data and dialog you're referring to. showReviewUI gets called when a user clicks "Review" for a file attachment (see screenshots).

tinayang Dec. 3, 2012, 10 p.m.

oops, sorry let me try this again. Please correct me if I'm wrong, but I thought the different review UI will be embedded in the body of the showReviewUI dialog? From interacting with the existing UI, as a user I would expect if there is unsaved comment in any review UI I would get a prompt before I leave the page reminding me that there is unsaved work, are you sure you want to leave this page? I was wondering if the review UIs have the same function? and if so when they are embedded in the showReviewUI dialog are they behaving the same way? I was worry that the close button would just remove the dialog html from <body> and you won't get a reminder. Does that make a bit more sense?

chipx86 Dec. 4, 2012, 12:47 a.m.

On that note, if you have a comment dialog open and close the editor, I imagine the dialog remains? We'll need to solve these issues.

reviewboard/static/rb/js/reviews.js (Diff revision 10)

I'm curious, what happens if the title (eg. caption) is longer than the width of the dlg?

aam1r Dec. 3, 2012, 7:40 p.m.

Good question. I tried it and it just continues over to the next line. I've added a screenshot.

tinayang Dec. 3, 2012, 10 p.m.

Thanks for checking! I think that looks okay.

reviewboard/templates/reviews/review_request_box.html (Diff revision 10)

I personally think it would be useful if the title also show the MIME type and/or filename of the file being reviewed, even if there is a caption. What do you think?

aam1r Dec. 3, 2012, 7:40 p.m.

I think it would be pretty neat actually to show the filename as well as the caption. If "README.md" had the caption "our updated README file", I could make it so that it's "README.md: our updated README file". It gives more context when they're viewing the UI and fills up that empty space in the title bar.

I am not sure though if that is consistent with the rest of the UI. 

Mentors: any thoughts?

chipx86 Dec. 4, 2012, 12:47 a.m.

Maybe just in parens. "My image (foo.png)"

Of course, if there's no caption, it should just show the filename still.

Another thing to consider is draft captions. If there's a draft object in the context and there's a file.draft_caption, that should be used. I think David just recently committed a change similar to that.

Added Files:

Change Summary:

Fixed issue that slchen brought up.

Diff:

Revision 11 (+50 -10)

Show changes

	reviewboard/reviews/urls.py
	reviewboard/reviews/ui/base.py
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/templates/reviews/ui/base_inline.html

I'm quite happy with this. Thank you Aamir.

Ship it!

```
Grr - keep forgetting to hit "Ship it".
```

Ship it!

```
Grr - keep forgetting to hit "Ship it".
```

reviewboard/reviews/urls.py (Diff revision 11)

The issue has been resolved. Show all issues

Exceeds max line length:

from reviewboard.reviews.ui.base import get_urlpatterns \
    as get_review_ui_urlpatterns

aam1r Dec. 4, 2012, 8:57 p.m.

Not sure how I feel about moving it to the next line since it's only 4 extra characters.

Mentors: any thoughts on whether it's a good idea to move this to the next line or is this okay?

david Dec. 4, 2012, 9:58 p.m.

You could just rename this get_review_ui_urls and get the best of both worlds.

chipx86 Dec. 4, 2012, 10:20 p.m.

I like that better.

But in general, yes, it should be wrapped, even on imports.

Change Summary:

Renamed "get_urlpatterns" to "get_review_ui_urlpatterns" which makes the
import statement shorter as we don't need an additional "as ..." part.

Diff:

Revision 12 (+50 -10)

Show changes

	reviewboard/reviews/urls.py
	reviewboard/reviews/ui/base.py
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/templates/reviews/ui/base_inline.html

Rendering pluggable UIs in lightbox

Information

Reviewers

Change Summary:

Description:

Diff:

Change Summary:

Testing Done:

Change Summary:

Screenshots:

Change Summary:

Description:

Testing Done:

Diff:

Screenshots:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Description:

Testing Done:

Change Summary:

Screenshots:

Change Summary:

Summary:

Description:

Testing Done:

Diff:

Change Summary:

Screenshots:

Change Summary:

Added Files:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Description:

Diff:

Added Files:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Discarded