Change Summary:
Added screenshot.
Testing Done: |
|
---|
Review Request #2224 — Created March 30, 2011 and submitted
Information | |
---|---|
kfquinn | |
Review Board | |
Reviewers | |
reviewboard | |
mike_conley |
I needed a drop down menu for an extension I was building so I built an extension to do just that. Very similar to the standard button extension, except it's a list of buttons essentially.
Manual testing using the extension I made.
reviewboard/extensions/hooks.py (Diff revision 1) |
---|
I think this is a little confusing/misleading. Developers might try to set options as a list of ActionHook objects. Maybe say, "a list of ActionHook-style dicts". I'm not sure if that's any clearer. :/
reviewboard/extensions/hooks.py (Diff revision 1) |
---|
The comment should be more like the one above. It should be clear from reading it exactly what it's purpose is. First line, on the """, in the form of a quick summary without "This is" or similar.
reviewboard/extensions/hooks.py (Diff revision 1) |
---|
I'm also confused about this. Also the "see above." What should be in here? Is it actually ActionHook instances?
reviewboard/extensions/templatetags/rb_extensions.py (Diff revision 1) |
---|
This must wrap to < 80 characters. Probably want one parameter per line. Also, spaces after ",".
reviewboard/templates/extensions/action_dropdown.html (Diff revision 1) |
---|
The image path is being hard-coded and doesn't contain a media serial. Please use: {{MEDIA_URL}}rb/images/dropdown.png?{{MEDIA_SERIAL}}
reviewboard/templates/extensions/action_dropdown.html (Diff revision 1) |
---|
No spaces before this. Instead, put spaces after the {% for indenting.
Style changes, adding example, using {{settings.MEDIA}}, commenting changes.
Diff: |
Revision 3 (+56 -3) |
---|
Committed with a few style fixes. Thanks! There are a few small question marks in my head about the bet way to implement this dropdown actionhook. I almost want a standard ActionHook to be able to supply sub-items. Also, I sort of think it should be more generic so it can be used in the diff viewer too. However, it's fine for now, and I think it's very likely we'll be revisiting how actionhooks work by the time we ship extensions in a release.