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.

CommentFileSizeAuthor
#37 block.easy.add_6.patch5.96 KBRobRoy
#28 block.easy.add_5.patch5.55 KBRobRoy
#25 block.easy.add_4.patch5.57 KBRobRoy
#20 block.easy.add_3.patch5.83 KBRobRoy
#18 block.easy.add_2.patch5.84 KBRobRoy
#11 block.easy.add_1.patch6.07 KBRobRoy
#10 block_add.patch6.07 KBSteven
#9 block_add_configure.patch5.68 KBHeine
#4 blocks_0.png79.45 KBRobRoy
#2 block.easy.add_0.patch8.24 KBRobRoy
#1 block.easy.add.patch8.24 KBRobRoy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented
Status: Active » Needs review
FileSize
block.easy.add.patch8.24 KB

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.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented
FileSize
block.easy.add_0.patch8.24 KB

Re-rolled to include new changes in HEAD.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented
Version: x.y.z » 5.x-dev

Correct version.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented
FileSize
blocks_0.png79.45 KB

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.

m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented

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.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Needs work

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.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented

@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!

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented

Referenced at http://drupal.org/node/98390.

Heine’s picture

Heine CreditAttribution: Heine commented
Status: Needs work » Needs review
FileSize
block_add_configure.patch5.68 KB

An alternative approach displays the block configuration form with a custom submit handler.

Steven’s picture

Steven CreditAttribution: Steven commented
FileSize
block_add.patch6.07 KB

Fixed bugs:

  • Form's title was '' block for new blocks.
  • The MySQL bid sequence needs to be set to the right value in the update.
  • PgSQL can keep on using serial as far as I know.

Needs to be tested, especially on pgsql. Be sure to clear the menu cache.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented
FileSize
block.easy.add_1.patch6.07 KB

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.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented
Status: Needs review » Reviewed & tested by the community

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.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented

P.S. sepeck told me to. :)

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented

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?

Dries’s picture

Dries CreditAttribution: Dries commented
Version: 5.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Needs work

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.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented

having to specify a fake delta

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?

and than special casing block_admin_configure() to deal with fake delta's

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.

if (isset($info[$delta])) {
    drupal_set_title(t("'%name' block", array('%name' => $info[$delta]['info'])));
  }
Also, this should wait for Drupal 6 IMO.

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. :)

Steven’s picture

Steven CreditAttribution: Steven commented

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.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented
Version: 6.x-dev » 5.x-dev
Status: Needs work » Needs review
FileSize
block.easy.add_2.patch5.84 KB

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

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

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.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented
FileSize
block.easy.add_3.patch5.83 KB

Good catch. I missed that the update code had that. Re-rolled.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented

Oh, and thanks for the catch pwolanin!

Steven’s picture

Steven CreditAttribution: Steven commented
Category: feature » bug
Priority: Normal » Critical

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.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented

Still applies with offset. I've tested this out and it works great. What's holding this up? The final word from Dries?

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Needs review » Needs work

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.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented
Status: Needs work » Needs review
FileSize
block.easy.add_4.patch5.57 KB

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.

BioALIEN’s picture

BioALIEN CreditAttribution: BioALIEN commented

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.

Dries’s picture

Dries CreditAttribution: Dries commented
Priority: Critical » Normal

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):

+function block_add_block_form() {
+  $form = block_admin_configure('block', NULL);
+  return $form;
+}
RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented
FileSize
block.easy.add_5.patch5.55 KB

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.

Playing tricks with the 'delta' is a bit of a hack but probably the simplest solution for now.

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?

chx’s picture

chx CreditAttribution: chx commented

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?

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented

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.

joshk’s picture

joshk CreditAttribution: joshk commented

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.

joshk’s picture

joshk CreditAttribution: joshk commented

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.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented

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.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented

Come on boys, let's get this in! Steven, any last words?

seanr’s picture

seanr
he/him
CreditAttribution: seanr commented

+1

joshk’s picture

joshk CreditAttribution: joshk commented
Status: Needs review » Reviewed & tested by the community

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!

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented
Version: 5.x-dev » 6.x-dev
FileSize
block.easy.add_6.patch5.96 KB

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!

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Alright! Committed to CVS HEAD. :-)

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented

Thank you. Thank you. Thank you. :D

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)