Posted by profix898 on June 21, 2007 at 3:08pm
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | block.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
We are currently listing all themes in the blocks administration and for every theme we have a tab named 'themeX settings'. If someone has many themes installed/enabled the tabs are quickly getting confusing. I suggest to remove the word 'settings' from the title. It makes the list much clearer without loosing context. Attached is a 'micro' patch which removes the word 'settings' (and adds a missing '$form_state').
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| block_settings_title.patch | 1003 bytes | Ignored: Check issue status. | None | None |
Comments
#1
+1 to removing that word 'settings'.
but your change to block_admin_display() does not look right. it is called from block_menu() with: page arguments' => array('block_admin_display', $key),
that means that $key will be first arg. also, you can simplify $title in that menu declaration and eliminate 'title arguments'
#2
OK. Simplified $title (and eliminated 'title arguments').
As for the change to block_admin_display(). I was not sure there either. $key should be the first arguments, yes, but actually it is not. $form_state is passed as first arguments and $key comes second. Without the 'fix' the themes in blocks administration dont switch, what means you cant configure blocks per theme (and it generates a warning). Might be a bug in the FAPI3 section though ... I removed that piece from the patch for now. Thanks.
#3
-1. Setting the title to the theme name is really confusing
#4
@dmitrig01: Because you think it's confusing, it doesnt mean that the 'code needs work'. Its just your opinion. Or is there stg wrong with the patch?
#5
@dmitrig01: Because you think it's confusing, it doesnt mean that the 'code needs work'. Its just your opinion. Or is there stg wrong with the patch?
#6
Still applies (with 4 lines offset). Anyone?
#7
Committed to CVS HEAD. Thanks.
#8