Rendering pluggable UIs in lightbox
Review Request #3526 — Created Nov. 17, 2012 and discarded
Information | |
---|---|
aam1r | |
Review Board | |
master | |
Reviewers | |
reviewboard | |
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. |
|
|
I probably screwed up, but there should be two blank lines. |
|
|
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 … |
|
|
When comparing against not, use "is" or "is not." In this case, though, don't do that comparison, just "if not … |
|
|
We already have this in mimetypes.py, so let's not duplicate it here. |
|
|
Separate these. Anything imported from reviewboard.* is in the "current project imports" group. "django" is in the "3rd party imports" … |
|
|
URLs should always end with "/". "fetch" is a weird URL. It's not likely what we want. Maybe "rendered." |
|
|
line too long |
JS jsintal | |
line too long |
JS jsintal | |
Should be aligned with "urlpatterns" |
|
|
Should be two blank lines. |
|
|
Alignment looks off, but no need to have one per line. Do you mean for this function to still be … |
|
|
Where did the original files go? Merging issue? |
|
|
This is global. This should all be handled by the ReviewUI subclass. The click handler should only call something on … |
|
|
The title should include the attachment display name or filename. |
|
|
Why are you limiting this to 500px? |
|
|
We just hit some problems with dynamic URL handling for extensions, and came up with a nifty thing called DynamicURLResolver. … |
|
|
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. |
|
|
Use 1-space indentation, and make sure parameters align. |
|
|
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: |
|
|||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+157 -7) |
Change Summary:
Added Testing information
Testing Done: |
|
---|
-
-
-
reviewboard/reviews/ui/__init__.py (Diff revision 2) I probably screwed up, but there should be two blank lines.
-
reviewboard/reviews/ui/base.py (Diff revision 2) 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) 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) We already have this in mimetypes.py, so let's not duplicate it here.
-
reviewboard/reviews/ui/markdownui.py (Diff revision 2) 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) URLs should always end with "/". "fetch" is a weird URL. It's not likely what we want. Maybe "rendered."
-
-
-
reviewboard/reviews/views.py (Diff revision 2) 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.
-
-
reviewboard/static/rb/js/reviews.js (Diff revision 2) 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.
-
reviewboard/templates/reviews/ui/markdown.html (Diff revision 2) The title should include the attachment display name or filename.
Change Summary:
Addressed most issues
Description: |
|
|||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||
Diff: |
Revision 3 (+973 -165) |
|||||||||||||||||||||||||||
Screenshots: |
|
-
-
reviewboard/reviews/ui/base.py (Diff revision 4) If you wanted to make this cuter, you could use a list comprehension.
-
Change Summary:
Addressed a few issues and removed 'urlpatterns' stuff from MarkdownReviewUI since it does not need it.
Diff: |
Revision 5 (+50 -8) |
---|
Change Summary:
Updated "Description" and "Testing Done"
Description: |
|
||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
Change Summary:
Updated description and testing done. Review request no longer in WIP.
Summary: |
|
||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||
Diff: |
Revision 6 (+52 -11) |
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) |
---|
-
-
reviewboard/reviews/ui/base.py (Diff revision 7) 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) 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) 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) 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) Use 1-space indentation, and make sure parameters align.
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: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+51 -11) |
-
-
reviewboard/reviews/ui/base.py (Diff revision 10) Since mimeparse is 3rd party, it should be with the 2nd group of imports, right under: from djblets.util.urlresolvers import DynamicURLResolver
-
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?
-
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?
-
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?
-
-
reviewboard/reviews/urls.py (Diff revision 11) Exceeds max line length: from reviewboard.reviews.ui.base import get_urlpatterns \ as get_review_ui_urlpatterns
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) |
---|