Review Request Drop Down U.I. Extension

Review Request #2224 — Created March 30, 2011 and submitted


Review Board


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.


  1. Kevin:
    This looks pretty good.  Two really minor comments below.
    I think ChipX86 or purple_cow should give this a once-over before it get's shipped - we're reusing some code here, and it might be more awkward than they'd like.
  2. reviewboard/extensions/ (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.  :/
  3. space before "actions"
  2. reviewboard/extensions/ (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.
  3. reviewboard/extensions/ (Diff revision 1)
    Shouldn't have so many spaces after the :
  4. reviewboard/extensions/ (Diff revision 1)
    I'm also confused about this. Also the "see above." What should be in here? Is it actually ActionHook instances?
    1. Options is a dict of action hooks.  The parameters are the same.
  5. One line.
    1. But.. every other method is commented in this fashion, should I change all of them?
  6. This must wrap to < 80 characters. Probably want one parameter per line.
    Also, spaces after ",".
  7. The image path is being hard-coded and doesn't contain a media serial. Please use:
    1. I tried this, but it doesn't seem to know {{MEDIA_URL}} & {{MEDIA_SERIAL}} is.
  8. No spaces before this. Instead, put spaces after the {% for indenting.
Review request changed
  1. 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.