How it could look:
[[Image(old.png)]]
[[Image(new.png)]]
--
Ticket URL: <https://code.djangoproject.com/ticket/19235>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
* needs_docs: => 0
* needs_better_patch: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
I agree with your premise that the number of actions generally tends to be
quite low (most of the time it's just "Delete selected"), that as a result
a drop-down list is cumbersome, and that buttons would be a saner default.
My recommendation for the implementation is as follows:
- rename the template `admin/actions.html` to
`admin/actions_as_select.html`. This change would be slightly backwards-
incompatible therefore some upgrade instructions need to be provided in
the release notes for the (I presume not many) developers who have
customized this template in their project.
- add a new template `admin/actions_as_buttons.html`. This presentation
would have to use the exact same API as for the select drop-down, i.e. via
POST requests, so that the two templates are interchangeable.
- add a `actions_template` attribute to `ModelAdmin`, defaulting to
`admin/actions_as_buttons.html`. This would allow developers to switch
between the two presentations as they see fit for each model. This would
also allow developers to provide an entirely different, custom
presentation.
--
Ticket URL: <https://code.djangoproject.com/ticket/19235#comment:1>
Comment (by marpetr):
I implemented the first two parts, but am struggling with the third one.
The problem is, template filename is hardcoded in
`contrib/admin/templatetags/admin_list.py`:
{{{#!python
@register.inclusion_tag('admin/actions.html', takes_context=True)
def admin_actions(context):
"""
Track the number of times the action field has been rendered on the
page,
so we know which value to use.
"""
context['action_index'] = context.get('action_index', -1) + 1
return context
}}}
The static code above does not have access to `ModelAdmin` object. I
thought about extending `inclusion_tag` to support dynamic filenames, but
it's a publicly documented function. The only solution I can think of now
is to create `dynamic_inclusion_tag` which would take the template
filename from tag keyword arguments. Then the filename would be passed to
the template in `ModelAdmin.changelist_view`:
{{{#!python
context['actions_template'] = self.actions_template
}}}
And then in `contrib/admin/templates/admin/change_list.html`:
{{{#!text/html
{% if action_form and actions_on_top and cl.full_result_count %}{%
admin_actions template=actions_template %}{% endif %}
{% result_list cl %}
{% if action_form and actions_on_bottom and cl.full_result_count %}{%
admin_actions template=actions_template %}{% endif %}
}}}
Is this a good plan?
--
Ticket URL: <https://code.djangoproject.com/ticket/19235#comment:2>
* status: new => assigned
* owner: nobody => marpetr
--
Ticket URL: <https://code.djangoproject.com/ticket/19235#comment:3>
Comment (by julien):
I think a cleaner implementation would be to turn `admin_actions` into a
`simple_tag` that would accept the `ModelAdmin` instance as parameter and
have the `takes_context` flag turned on. Then the logic for rendering the
appropriate template could occur inside the `admin_actions` function's
body instead of via the decorator's parameter. Does that make sense?
--
Ticket URL: <https://code.djangoproject.com/ticket/19235#comment:4>
Comment (by marpetr):
I have uploaded the patch. Short summary:
* Added `ModelAdmin.actions_template` field. It can be
`admin/actions_as_select.html` or `admin/actions_as_buttons.html`
(default)
* Replaced a big chuck of code in `ModelAdmin.changelist_view` by
functionally equivalent code. Separate handling of actions with and
without confirmations does not make sense, and the empty selection check
is also done in `ModelAdmin.response_action`.
* Updated `actions.js` to work with button layout and recompressed
`actions.min.js`. (By the way, recompressing the other three files changes
the result slightly. Someone should update them)
* Renamed `admin/actions.html` to `admin/actions_as_select.html`.
* Wrote `admin/actions_as_buttons.html`. The `{% if choice.0 %}` part
skips the blank choice '-------'.
* Updated changelist template and `admin_actions` template tag as you
suggested above.
Could you check if this is right? I tested every change manually,
including `list_editable`. I still have to run the full test suite.
--
Ticket URL: <https://code.djangoproject.com/ticket/19235#comment:5>
* needs_docs: 0 => 1
* has_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
The patch is looking good. Could you please add some tests to make sure
the `actions_template` attribute's value generates the correct output? The
documentation would also need to be updated. Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/19235#comment:6>
* needs_tests: 1 => 0
Comment:
I added one test which verifies that button layout works and fixed two
tests which relied on select layout.
I also added the docs for `ModelAdmin.actions_template` field.
The only things left are to update release notes (I don't know if 1.5 or
1.6) and update the screenshots (I don't have a Mac).
--
Ticket URL: <https://code.djangoproject.com/ticket/19235#comment:7>
Comment (by marpetr):
Could this patch still get into 1.5?
--
Ticket URL: <https://code.djangoproject.com/ticket/19235#comment:8>
Comment (by julien):
@marpetr: This will get in 1.5 if the patch can get finalized before the
beta release.
The patch is looking good. I've made the following changes:
* added 1.5 release doc.
* set `actions_template` to `None` by default to be consistent with other
similar `ModelAdmin` attributes.
* factored the selection counter HTML into a separate included template.
The main thing that needs a bit of work now is the admin actions doc
(https://docs.djangoproject.com/en/dev/ref/contrib/admin/actions/). Could
you have a go at this?
Also, I haven't looked in detail into your refactoring of
`changelist_view` yet. Could you elaborate a bit here on the changes
you've made?
--
Ticket URL: <https://code.djangoproject.com/ticket/19235#comment:9>
Comment (by marpetr):
@julien: For some reason I cannot apply the patch, so I could only have a
look at it.
1. I think it would be better to use `{% for key, value in
action_form.fields.action.choices %}` in `actions_as_buttons.html`
template to avoid weird-looking `{{ choice.0 }}` identifiers.
1. About the `changelist_view` refactoring:
the old action handling code separated two cases: actions with
confirmations (eg. delete selected objects) and actions without
confirmations. The type was determined by checking if `POST['index']` is
set. Now, in `actions_as_buttons` layout we no longer need
`POST['index']`, because there will always be only one `POST['action']`
value. Furthermore, the difference in handling actions with and without
confirmations is: if the action is **with** confirmation (eg. it's the
second step of delete action) and there are no selected objects, no error
message will be shown. It does not make sense to me -- how could you have
proceeded to the second step without selecting any objects? Also, this
code assumes that `changelist_view` action form will always set
`POST['index']` and any other forms will never set it, which is
undocumented behavior. Let's say we get rid of this distinction, then we
have:
{{{#!python
if actions and request.method == 'POST' and '_save' not in request.POST:
if selected:
response = self.response_action(request,
queryset=cl.get_query_set(request))
if response:
return response
else:
action_failed = True
else:
msg = _("Items must be selected in order to perform "
"actions on them. No items have been changed.")
self.message_user(request, msg)
action_failed = True
}}}
This code is redundant because `response_action` method performs this
check itself. If there are no selected items, `response_action` will show
the same message to the user and return None. So the code can be further
simplified to:
{{{#!python
if actions and request.method == 'POST' and '_save' not in request.POST:
response = self.response_action(request,
queryset=cl.get_query_set(request))
if response:
return response
else:
action_failed = True
}}}
Finnally, `action_failed` flag is only used in the following check:
{{{#!python
if (request.method == "POST" and cl.list_editable and
'_save' in request.POST and not action_failed):
}}}
However, conditions `'_save' in request.POST` and `'_save' not in
request.POST` are mutually exclusive so we don't need `action_failed` flag
at all. I hope this all makes sense.
3. Admin actions document does not contain any verbal references to
action interface being select, dropdown or combobox, so only the
screenshots need to be updated. As a last thing, could you add this right
above `Advanced action techniques` in `admin/actions.txt`: (I'm sorry
again that I can't do this myself)
{{{
Customizing the appearance of action bar
----------------------------------------
By default the action bar is rendered as a list of buttons, however a
drop-down list
interface may be more suitable when the number of actions is large. You
can control
this behavior by setting :attr:`ModelAdmin.actions_template` attribute:
class ArticleAdmin(admin.ModelAdmin):
...
actions = ['first_action', 'second_action', 'third_action',
'fourth_action', ...]
actions_template = 'admin/actions_as_select.html'
...
.. attribute:: ModelAdmin.actions_template
.. versionadded:: 1.5
Path to a template used for rendering the changelist's action bar.
Django
provides two templates: ``admin/actions_as_buttons.html`` (default)
and ``admin/actions_as_select.html`` (used to be the default in
version 1.4
and below).
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19235#comment:10>
* status: assigned => closed
* resolution: => fixed
Comment:
This was resolved by #27728, which allowed overriding admin templatetag's
templates.
--
Ticket URL: <https://code.djangoproject.com/ticket/19235#comment:11>