Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor ButtonHelper component of modeladmin to offer drop-down/sub-menu structuring of actions #7

Open
ababic opened this issue Jun 20, 2016 · 4 comments
Labels
type:Enhancement New feature or request

Comments

@ababic
Copy link

ababic commented Jun 20, 2016

The new more/dropdown UI element used in the Explorer to hide less common actions is really smart, and would certainly be a pattern worth reusing. A logical first step would be look at what's there, and what can be abstracted out for re-use, then to refactor the ButtonHelper component to make use of it.

At the same time, it would probably make sense to refine the way buttons in ButtonHelper are defined (it could be more DRY than it is) and rendered in the various modeladmin templates.

@gasman gasman added the type:Enhancement New feature or request label Jun 22, 2016
@ababic
Copy link
Author

ababic commented Jul 29, 2016

After having time to think this through, I have a proposal for what I'd like to achieve here:

Currently, adding a new button for items in the IndexView (or elsewhere) requires the ButtonHelper class to be sub-classed, and methods adding / overriding in that class. One method needs to define the button (it's text, where it links to etc), and another method does some permission analysis as it bulks buttons together.

I would like to make this process easier by...

  1. Allowing additional button methods to be defined on the ModelAdmin class itself - much like ModelAdmin allows you to define methods that can be named in list_display and accessed at the appropriate time. The ButtonHelper can simply look for a 'definition method' on the ModelAdmin class if it doesn't have a method of it's own.
  2. Add attributes to the ModelAdmin class allowing 'button sets' to be defined, using a simple iterable, e.g:
index_view_buttons = ('edit', 'inspect', 'delete', 'copy')

The appropriate iterable can then be passed to the ButtonHelper's get_buttons_for_obj method, which can then find definitions for each of those buttons, and return an appropriate 'structure' for rendering. I use the term 'structure', because I also hope to support more complex iterables for these 'button series' attributes, to utilise the 'drop-down/sub-menu' design pattern, like so:

index_view_buttons = (
    'edit',
    'view',
    'add_child',
    ('more', ('move', 'copy', 'delete', 'unpublish', 'revisions'),
)

Which would reproduce the menu shown in Explorer.

What I have yet to decide on, is how best to bring permissions into the equation... so that the right buttons only show for the right users. I suppose the simplest solution there would be for each 'button description' method to include a permissions_required iterable within the dictionary they return, which could then be passed off to PermissionHelper to figure out whether the button should be included in the final structure.

@gasman Interested to hear your thoughts on this :)

@ababic
Copy link
Author

ababic commented Feb 21, 2017

There's another piece of work being discussed in #10 which looks like it will benefit from a few of the features discussed above, but the support for multi-level/dropdown definitions probably isn't as important as the rest of the stuff.

For now, I think we just need to focus on the following, and work on bringing in the dropdowns later:

  1. Add an index_view_buttons attribute that let's you define a flat list of the buttons you want to show in IndexView for each row, e.g.
index_view_buttons = ('edit', 'inspect', 'delete', 'copy')
  1. Add an inspect_view_buttons attribute that let's you define a flat list of the buttons you want to show at the bottom of the InspectView page, e.g.
inspect_view_buttons = ('edit', 'delete', 'copy')
  1. Support the definition of 'button definition methods' on the ModelAdmin class instead of just on the ButtonHelper, so that you don't have to subclass ButtonHelper just to define a new button or override what an existing one does.

  2. (The difficult bit) Figure out how to make ButtonHelper respect the new attributes / button definition method locations while preserving backwards compatibility.

@ababic
Copy link
Author

ababic commented Feb 26, 2017

I think I've figured out how to implement these changes in a backwards compatible way:

We leave the existing ButtonHelper classes largely as they are, but make them raise a deprecation warning on init(). That way, anyone subclassing them will be fine for now.

We create a new GenericButtonHelper class that has the new behaviour on it, and update the ModelAdmin class to use that by default. But, anyone using the button_helper_class attribute to use a custom class (based on one of the existing classes) won't have to worry, as that will be respected.

Then it's just a case of making sure the views support both the old and new style classes when they call on them to do button-related things.

@ababic
Copy link
Author

ababic commented Mar 22, 2017

Quick update: I've made a huge amount of progress with this (including getting the dropdown buttons to render), but it ended up requiring changes in a few more places than I was expecting, and so I'm thinking it'll now be best to split the changes up over a few different PRs to make things easier to review (with unit tests added along the way as required)

  • Better code separation (See Improved code separation in contrib.modeladmin wagtail/wagtail#3467)
  • Update URLHelperto support fetching of URLs for more page actions
  • Update PermissionHelper with has_methods_to_check_for_action and user_has_permission_for_action methods to allow checking of permissions without having to know specific method names
  • Deprecate the current ButtonHelper and PageButtonHelper classes and introduce the GenericButtonHelper class and new methods on the ModelAdmin class for controlling which buttons appear where, and with what attributes values (still big, but not as widespread)

In case I die or something, here's where things are at:
wagtail/wagtail@master...rkhleics:new-button-helper

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants