Rendering pluggable UIs in lightbox
Review Request #3526 — Created Nov. 17, 2012 and discarded
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).
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.
| Description | From | Last Updated |
|---|---|---|
|
Probably should be the attachment's display name, or filename if that's not there. |
 chipx86
|
|
|
I probably screwed up, but there should be two blank lines. |
chipx86
|
|
|
continuation line over-indented for hanging indent |
JS jsintal | |
|
Since this is a function, it should be prefixed with get_() I think actually, just get_urlpatterns, since the context should … |
chipx86
|
|
|
When comparing against not, use "is" or "is not." In this case, though, don't do that comparison, just "if not … |
chipx86
|
|
|
We already have this in mimetypes.py, so let's not duplicate it here. |
chipx86
|
|
|
Separate these. Anything imported from reviewboard.* is in the "current project imports" group. "django" is in the "3rd party imports" … |
chipx86
|
|
|
URLs should always end with "/". "fetch" is a weird URL. It's not likely what we want. Maybe "rendered." |
chipx86
|
|
|
line too long |
JS jsintal | |
|
line too long |
JS jsintal | |
|
Should be aligned with "urlpatterns" |
chipx86
|
|
|
Should be two blank lines. |
chipx86
|
|
|
Alignment looks off, but no need to have one per line. Do you mean for this function to still be … |
chipx86
|
|
|
Where did the original files go? Merging issue? |
chipx86
|
|
|
This is global. This should all be handled by the ReviewUI subclass. The click handler should only call something on … |
chipx86
|
|
|
The title should include the attachment display name or filename. |
chipx86
|
|
|
Why are you limiting this to 500px? |
 david
|
|
|
We just hit some problems with dynamic URL handling for extensions, and came up with a nifty thing called DynamicURLResolver. … |
chipx86
|
|
|
Blank line between these. Third-party imports and current project imports must be in their own import groups. |
chipx86
|
|
|
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
|
|
|
I was confused at first about which 'this' we were using. Maybe instead of passing the link, pass the URL. |
chipx86
|
|
|
Use 1-space indentation, and make sure parameters align. |
chipx86
|
|
|
Since mimeparse is 3rd party, it should be with the 2nd group of imports, right under: from djblets.util.urlresolvers import DynamicURLResolver |
SL slchen | |
|
Exceeds max line length: from reviewboard.reviews.ui.base import get_urlpatterns \ as get_review_ui_urlpatterns |
SL slchen |
- 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)
- 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
-
-
-
-
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.
-
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.
-
-
Separate these. Anything imported from reviewboard.* is in the "current project imports" group. "django" is in the "3rd party imports" group.
-
URLs should always end with "/". "fetch" is a weird URL. It's not likely what we want. Maybe "rendered."
-
-
-
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.
-
-
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.
-
- 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)
- Screenshots:
-
Caption shown as title (when available)Filename shown as title as a fallback
- Change Summary:
-
Addressed a few issues and removed 'urlpatterns' stuff from MarkdownReviewUI since it does not need it.
- 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 lightboxRendering 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.
- Change Summary:
-
Removing screenshots to add new ones
- Screenshots:
-
Caption shown as title (when available)Filename shown as title as a fallback
- 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.
-
-
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. -
Blank line between these. Third-party imports and current project imports must be in their own import groups.
-
It'd be better to do ".... import get_urlpatterns as get_review_ui_urlpatterns", to make it clear what it is we're accessing.
-
I was confused at first about which 'this' we were using. Maybe instead of passing the link, pass the URL.
-
- Change Summary:
-
Removed accidentally committed file that wasn't part of this commit.
- Diff:
-
Revision 9 (+52 -12)
- 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.
-
Looks great Aamir! Looking forward to use this with the inline file attachments UI.
-
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?
-
-
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?
