From README.TXT:

Allows the developer and administrator to move blocks to other regions and
positions.

Drupal blocks have a fixed region and position on all pages in the website.
This module allows the developer to make exceptions based on the current page
or situation (per request), and allows website administrators to 'drag & drop'
movable blocks to other regions or positions on specific pages, or hide blocks
on specific pages. This module adds configuration options to the block settings,
in which can be configured to which regions the block can be moved to.

This module seems a bit like Homebox but is different because the exisiting regions are used instead of a separate specific Homebox layout. It is valuable if block positions should be different in some situations; for example if on a 'news' page the content should be in the sidebar except in the main content region. Possibilities are endless. It is also handy if the website administrator has the desire to move blocks on pages, for example because they want to balance the layout better.

Project page: https://drupal.org/sandbox/bvanmeurs/1702554
Git: git clone --recursive --branch 7.x-1.0 http://git.drupal.org/sandbox/bvanmeurs/1702554.git block_position

To try it out:
Install the module.
Configure some blocks to be movable in the block edit screen.
Try moving them around the page.
Reload the page, and you will see that the configuration is applied.

Thanks to anyone who wants to take the time to review this module!

CommentFileSizeAuthor
block_position.jpg111.59 KBbvanmeurs

Comments

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work

Hi !
The general idea of this module is very interesting, however I have a few remarks

1) I moved my block from one region to another but then I couldn't move it back, did I do something wrong ? And more importantly, I couldn't set it back to it's previous position because for the block module the block was still in its original position. You have to do something about that.

2) It is weird that the position is saved automatically, it should be asked to save the config first.

3) You're README file is not very precise about how to start using the module

I'll take a better look at the code after you've corrected the above and I feel like it is usable.

patrickd’s picture

patrickd commented
Status: Needs work » Needs review

none of your remarks are major issues, therefore, please don't block further reviews.

marshmn’s picture

marshmn commented

Hi,

From a manual review of the code in this module:

  • There are some instances of strings which should be enclosed within t() but which are not - for example, in block_position_menu() the title & description should be translatable
  • The module sets some variables - these should be removed on uninstall by implementing hook_uninstall() and using variable_del() within that to remove them
  • You seem to have a branch called 7.x-1.0 but I believe you should be working in a branch called 7.x-1.x; additionally I think you have a master branch which should be removed - this may help: http://drupal.org/node/1015226
patrickd’s picture

patrickd commented

DO NOT USE t() WITHIN hook_menu() ! DON'T!

marshmn’s picture

marshmn commented

Ah, my bad for advising to use t() in hook_menu()... I've just been and read the documentation and yes, I see that I was wrong in suggesting that... apologies for the confusion...

As patrickd says, don't use t() in hook_menu().

bvanmeurs’s picture

bvanmeurs commented

Quentin, thanks for your comments and ideas. You should be able to drag & drop the block back in the same way as you first moved it. What browser are you using?

Saving is automatic because it is more user-friendly. In most drag and drop applications including Homebox, this is the standard. But I agree that it sould be possible to reset the original position, and give some feedback about saving. This is certainly on the roadmap, but not for this release.

Marshmn, I will add the deletion in the uninstall hook. The master branch exists but only contains a readme.txt file with instructions to use another branch. You are correct about the branch name, so I will change that.

I will go on holidays today but will be back in 2 weeks. Thanks for your effort, I hope you can re-review after the changes.

Regards,
Bas

sanchi.girotra’s picture

sanchi.girotra commented

Manual Review:
In .module file, you have used variable "block_position_page_settings_%" so please uninstall them using hook_uninstall().

drupaldrop’s picture

drupaldrop commented
  • It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

    • The following git branches do not match the release branch pattern, you should remove/rename them. See http://drupal.org/node/1015226

  • On Installing a module. I face one issue . I have open my test site in two browsers chrome(18.0) and firefox(7.0.1). I move the block in one browser to some other region and it works , but when i refresh the same page on the other browsers that block moves to its original position from where it is moved earlier.

    • Please recheck.

Anonym’s picture

Anonym commented

Hello.

  1. block_position.module, lines 189-190:
    $json = 'var movable_blocks_path = ' . drupal_json_encode($page_path) . ';';
drupal_add_js($json, array('type' => 'inline'));

    Please use 'type' => 'settings'. Global variables is not good.

  2. block_position.module, line 205:
    foreach ($blocks as $bid => $block) {...}
    Unused variable $bid.
  3. block_position.module, line 349:
    drupal_set_message(t("There is a contradiction in the block overrides: @overrides", array('@overrides' => var_export($add_overrides, TRUE))), 'error');
    Uninitialized variable $add_overrides.
  4. block_position.api.php, line 69:
    if (strpos($original_path, 'search/')) {...}
    Uninitialized variable $original_path.
  5. movable_blocks.js:
    Please save $(document.body), $(document), $('.region'), $('.movable-target') and other into variables for multiple usage.
bvanmeurs’s picture

bvanmeurs commented

Thanks for your comments. I created the uninstall hook and made changes according to Anonym's review. Great that you spend some time on my module, and made some very helpful suggestions!

drupaldrop, I am not working in the master branch. The master branch only contains one file 'README.txt', as it should. The branch 7.x-1.0 contains the source code. Please check this page if you are not convinced: https://drupal.org/node/1702554/git-instructions/7.x-1.0/nonmaintainer

You are correct that on a page refresh, you should see the changes directly in the other browser. I am not sure why it doesn't work in your case. The module saves the new block positions via JSON, and it uses javascript for this. In my case, this request is handled within .5 seconds, but maybe it takes longer on your website? Can you please try to check a developer toolbar and see what happens to the request, and check if the changes are applied after the request was completed?

Thanks!

Bas

Milena’s picture

Milena commented

Hello,

Would you mind explaining how your module differs from context or panels module on your project page?
This three modules allows you to change order, region and some more things on different context-basis. It is good to show people what differences they have and when they should choose your module over others.

Your description in README.txt is not so good. I had no idea for a while what to do to make your module work. You should consider placing there "Try it out" section from Application Issue.

Functionality

It is hard to check many or all of the regions on block configuration page. I would suggest you to add check all button (or make all regions checked by default like context in some way).

Maybe it would also be good to place all form items from your module in a vertical tabs like the other options.

For some reason I'm not able to move the block. I've checked multiple regions and try to move the block on my front page and it just does not. There is no messages in my console. But it might be because of other sandbox modules.

About the master branch. I was instructed that default branch will do the thing, so we can remove master branch. I think documentation page is a bit unclear with that so it probably needs to be changed.

.info file

You should not create own group (based on company name I believe), especially the one that does not tell what's inside it. Creating new group is good when you provide more modules for chosen functionality like context or views. Group always should provide information what kind of modules it has.

.module file

You have a comment that tells: Specific Blauwhoed functionality.
What's Blauwhoed?

I believe your module has security issues (as had mine on the beginning). If I post a $_POST values through CURL your module will move blocks, because you do not check for security token. Greggles posted me how to prevent it (http://drupalscout.com/tags/csrf) and I believe you should also look into it.

In one of your foreach structure you use syntax:
foreach ($movable_blocks as $key => $value) { and do not use $key value.

What's more you put all data into the variables. They are store into memory on the beginning of each bootstraping Drupal. If I will be really patient (or mad) and customize blocks on many pages RAM memory can go out. Maybe, even though it can be a bit slower, getting data from database (own tables, altered tables) will be better?

Milena’s picture

Milena commented
Status: Needs review » Needs work
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.