Simplify blocks administration - Ability to specify visibility settings when adding a block
RobRoy - November 2, 2006 - 20:26
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | block.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
Right now when adding a block, users do not see the visibility options for that block. This is very confusing for new users who don't know they must go back and click 'configure' to modify vis settings.
We should also have the region dropdown (and enabled checkbox until http://drupal.org/node/91906 gets in) on the block add/edit page so we can do this in all one swoop.
Right now you have to
1) Add a block
2) Find the block in the list and go back to the same screen in #1 by clicking configure and change vis settings
3) Then find the block again in the list to enable/choose region.
Starting the issue so I remember to roll a patch.
#1
Okay, this was a bit harder than I expected, but I've got a working version here. You can finally add a block using the same form as when you configure one. No need for new users to have to 'know' to add a block, save, find in the list and hit configure again to get all these options. Much clearer.
I'd looove for this to get into 5.0 as I think it's a great improvement to the block admin interface along with kkaefer's patch. I did take out a chunk of redundant code that can be put back in for a release cycle if that is a big deal as it won't hurt anything, namely block_box_form_validate(), block_box_form_submit() which are useless now that we are using the configure form callbacks to handle adds/configures. Also, {boxes} now uses sequences and I covered this in system.install, but I need someone to check that Postgres update.
#2
Re-rolled to include new changes in HEAD.
#3
Correct version.
#4
Added a screen shot. It's the same as the edit blocks page, but now we can do it while adding blocks! No more confusion.
#5
I think this is a great idea and I face this problem constantly, great idea RobRoy!
+1 for the idea
Don't have time to look into the patch thoroughly right now but subscribing for updates.
#6
Personally, I'm not a fan of how this patch abuses the block hook -- it is a rather obscure practice, although it saves us some code. I think we'll want to work on a more elegant implementation that still re-uses code.
In general, this is too big of a change to include in Drupal 5.0.
#7
@Dries, when you say this abuses the block hook, are you referring to adding a return value for hook_block('save')? What are the potential negative implications of this? Let me know any specifics and I'll try to work on them.
Also, "it is a rather obscure practice, although it saves us some code." Could you elaborate on this "obscure practice" I'm not sure I follow. Thanks!
#8
Referenced at http://drupal.org/node/98390.
#9
An alternative approach displays the block configuration form with a custom submit handler.
#10
Fixed bugs:
'' blockfor new blocks.bidsequence needs to be set to the right value in the update.Needs to be tested, especially on pgsql. Be sure to clear the menu cache.
#11
I removed one extra space and made $delta a mandatory argument since we removed the code to check if (isset($delta)). This approach is much cleaner as I know see what Dries was saying about sacrificing some code duplication for more readable code. This works with me and should get into 5.x.
#12
Setting RTBC. I know I posted the last patch but it was a very minor change so I'm not really RTBC'ing my own patch...right? This is a big-time usability improvement and needs needs needs to get into Drupal 5 IMVVVHO.
#13
P.S. sepeck told me to. :)
#14
I know this is marked a feature request, but this is also a big usability bug and with all the great usability enhancements in D5, it would be a real shame not to get this in. Dries, can you take a look and see if the latest patch (that has been looked at by Heine and Steven) is up to snuff?
#15
The patch got better, but it is still a bit hack-ish IMO. For example, having to specify a fake delta and than special casing block_admin_configure() to deal with fake delta's. It doesn't make for readable code.
Also, this should wait for Drupal 6 IMO.
#16
Do you mean the fact that the add form has delta == 0 and then we overwrite that with the next seqid in the _submit()? menu_edit_item_form() does this and even though mid == 0 means less it's still the master root mid, right? My patch set $delta = NULL for the add form which is maybe a tad better. Is this criticism because any form_alter-injected validate/submit callbacks may think that this delta is talking about the actual block-0 not a fake added one. What would you recommend instead?
Do you mean this code to set a more apt title? We could easily remove this and just have the menu item's title be t('Edit block') if you've got a big problem with it, but I think we should leave it. Maybe I'm missing your point.
<?phpif (isset($info[$delta])) {
drupal_set_title(t("'%name' block", array('%name' => $info[$delta]['info'])));
}
?>
Of course I think this is thaw-worthy as it's so close and so important for blocks usability (it seems like other worthier people agree too :P), but of course...you're the man and I respect whatever you decide. :)
#17
I disagree with Dries... in Drupal 5.0 now, adding blocks is incredibly confusing and difficult. It needs to be fixed.
Will look at the code later.
#18
That's music to my ears brother. Marking 5.x not to be a pain in the ass, but just so this gets one last look.
I've re-rolled the patch and added a bit of Doxygen over the system_update_X() call. This still works great and I'll say it again, this belongs in 5.0! =D
#19
I like the general thrust, but why does the patch alter the bid column in the boxes table from int to tinyint? This seems like it undoes a recent bugfix.
#20
Good catch. I missed that the update code had that. Re-rolled.
#21
Oh, and thanks for the catch pwolanin!
#22
Raising this on the radar.
Having the "add" form for a block be a completely stripped down version of the "edit" form goes against everything we have in Drupal. This is a bug that should've been caught in the block configuration reforms, and the kind of usability goof that should put red on the cheeks of everyone involved.
#23
Still applies with offset. I've tested this out and it works great. What's holding this up? The final word from Dries?
#24
This patch changes quite a lot. Things like the database change and helper function changes should be split off so each part can get a proper review.
It may not be realistic to get this large of a change in Drupal 5 a this point.
#25
The db change and form changes are interconnected, so I don't think they should be split apart as that would make reviewing harder IMO. Here is a re-roll as the update_X needed an increment.
#26
I must agree with the general critics with regards to the block system. I've raised my fair share of issues with it and suggestions (tracker my 1st post). It's a part of Drupal that still needs some work and it seems to be getting some usability attention right here.
+1 for the efforts to get this into Drupal 5.
#27
Sorry but this is not a critical bug. Critical bugs prevent Drupal from working.
Playing tricks with the 'delta' is a bit of a hack but probably the simplest solution for now. :/ Not sure this should be committed as is.
The following function can be written shorter (or can be eliminated):
<?php+function block_add_block_form() {
+ $form = block_admin_configure('block', NULL);
+ return $form;
+}
?>
#28
Here's a patch that just does return block_admin_configure('block', NULL); I had a solution above that consolidated the form into one and you said (and I agreed) that it was a bit convoluted and would prefer some duplication over a cleaner method. This way is much cleaner and there is only a little duplication. We could remove that function and pass a #base element as a param to dynamically tell the form which validate/submit to go to, but this only adds 3 lines of code and is much more visible.
Yeah, this seems to be the best way we have at the moment.
Everything else is the same in this patch, just removed setting the $form needlessly. Look good enough?
#29
I am vehemently against a DB change at this point of the release cycle (of course if something is utterly broken that's something else, but this is only bothersome). And even if the powers that be overrule me, where are the pgsql changes?
#30
It seems pgsql doesn't need changing...see http://drupal.org/node/92630#comment-157490
From a usability standpoint this is utterly broken for new users IMHO. For you and me it is only bothersome, for new users this behavior is totally borked.
#31
I was going to submit a separate bug to point out that it was impossible to set the block title (let alone visibility) when creating a new block, which is a step backwards from 4.7. With the caviat that I haven't reviewed the scope of the patch (changing the DB schema sounds out of scale with the problem) the functionality gets a +1 from me.
#32
The DB change to use drupal sequences rather than auto increment is probably a good thing in the long run, but seems unrelated to the problem/solution here.
#33
Actually the db change is related to this change here as it's needed to get the next box ID upon adding a new block. This is how we're able to add the vis/other settings to the blocks/blocks_roles table on box creation.
#34
Come on boys, let's get this in! Steven, any last words?
#35
+1
#36
Patch applies clean vs current HEAD and works as advertised. Only thing better would be the ability to enable and place the block in a region w/weight. But this is good. Let's commit it!
#37
The system update needed a new number. Same patch re-rolled against HEAD.
This has had plenty of reviews, let's get this in to D6!
#38
Alright! Committed to CVS HEAD. :-)
#39
Thank you. Thank you. Thank you. :D
#40