Opened 14 years ago
Closed 11 years ago
#13629 closed New feature (fixed)
Admin Changelist: add app-model_name class to <body> tag
Reported by: | Łukasz Korzybski | Owned by: | tcsorrel |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | yes |
Description (last modified by )
Change form pages have "app-model_name" classes in body tag, for eg:
<body class="main-client change-form">. Change list pages don't, which
makes them unrecognizable in JavaScript code.
Example use case:
I'm currently implementing jQuery tooltips for searchbars to give user
information about which model's fields are searchable in each change list.
Currently to achieve this I had to override ModelAdmin.changelist_view (to
add model._meta to context) and change_list.html template. This is ok but I
think it would be nice to have it already done in django?
Attachments (2)
Change History (25)
comment:1 by , 14 years ago
Description: | modified (diff) |
---|---|
Triage Stage: | Unreviewed → Accepted |
by , 14 years ago
Attachment: | 0001-Closes-13629-add-app-model_name-class-to-body-tag.patch added |
---|
comment:2 by , 14 years ago
Has patch: | set |
---|---|
Needs tests: | set |
comment:3 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
comment:4 by , 14 years ago
Easy pickings: | set |
---|---|
Patch needs improvement: | set |
comment:5 by , 14 years ago
Scrap my comment about opts.object_name.lower
, which is fine to use here. I'd still like to see this new feature applied to all relevant admin templates though.
comment:6 by , 13 years ago
Owner: | changed from | to
---|---|
UI/UX: | unset |
comment:7 by , 13 years ago
Patch needs improvement: | unset |
---|---|
Resolution: | → fixed |
Status: | new → closed |
Version: | 1.2 → 1.3 |
I've updated the code to work with django 1.3. Also I've added more classes.
Class added:
- In home: dashboard home
- In application model list: dashboard app_label
- In change form: app_label-modelname change-form
- In change list: app_label-modelname change-list
- In delete confirmation: app_label-modelname delete-confirmation
- In delete selected confirmation: app_label-modelname delete-confirmation delete-selected-confirmation
by , 13 years ago
Attachment: | more-classes-in-admin.patch added |
---|
comment:8 by , 13 years ago
Needs documentation: | set |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
UI/UX: | set |
This isn't fixed until it has been checked in ;)
comment:9 by , 13 years ago
Owner: | removed |
---|---|
Status: | reopened → new |
comment:10 by , 12 years ago
Needs tests: | unset |
---|
I applied the more-classes-in-admin.patch patch and added a unit test to regressiontests/admin_views/test.py called testAppModelNameInClassForBodyTag(). I submitted a pull request for this.
comment:12 by , 12 years ago
Patch needs improvement: | set |
---|
Thanks for your work. I've left some specific comments on the pull request. Could you take a look and update it? Thanks!
comment:13 by , 12 years ago
Owner: | set to |
---|
comment:14 by , 12 years ago
Status: | new → assigned |
---|
New pull request can be found here : https://github.com/django/django/pull/575
comment:15 by , 11 years ago
Patch needs improvement: | unset |
---|
Patch is updated with last test done in the same pull request
comment:16 by , 11 years ago
Needs documentation: | unset |
---|
I don't know how and where this ticket should be documented
comment:17 by , 11 years ago
Patch needs improvement: | set |
---|
Thanks a lot for the updated pull request!
I think the patch looks pretty good. I just have one main comment: I'm not sure Selenium is really needed for the last test. Selenium tests are usually quite slow to run, so if there's a way to use the dummy client instead, then that would be preferable. See maybe if you can find similar examples in admin_views.tests.AdminActionsTest
.
Thanks again!
comment:18 by , 11 years ago
Thanks for the advice on sending post request more simply.
I changed the test client. And took also the opportunity to
- remove prefix redundant with assertContains message
- limit the test to check the body class app and model tag
comment:19 by , 11 years ago
Patch needs improvement: | unset |
---|
comment:20 by , 11 years ago
Thanks a lot Thomas! Could you please update the pull request? I'll review your changes as soon as I can. Thanks!
comment:21 by , 11 years ago
Thank you for updating the pull request. I think everything looks great. I just have 2 minor comments before merging your work. Could you please move the tests to the CSSTest
class inside admin_views/tests.py
, and could you make sure there are no trailing white spaces in the patch? Thank you!
comment:22 by , 11 years ago
Version: | 1.3 → master |
---|
Tests are moved CSSTest.
Trailing spaces are removed.
The ticket changes is rebased on master inside one commit.
Tests are passed.
comment:23 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
This is a good idea but while we're at it let's do it in every admin template, not just the change list. Also, using
opts.module_name
is preferable overopts.object_name.lower
.