This module allows the blocks to move between regions so fast, without having to enter the path: admin/structure/blocks.

Project Page: http://drupal.org/sandbox/sdramos/1737204
Git: git clone http://git.drupal.org/sandbox/sdramos/1737204.git drag_and_drop_block

Core: Drupal 7

Comments

Anonymous’s picture

Anonymous (not verified) commented

Hi !

For starter you are not supposed to include a license file. It will be added automatically by drupal. You also don't need to include the following code in your .info file. It will also be added automatically. 

version = "7.x-dev"
core = "7.x"
project = "drag_drop_block"
datestamp = "1305135114"

Review of .module file :
1. Functions drag_drop_blog_loadconfig() and drag_drop_block_save_order() might benefit from a short documentation.
2. Line 17 your comment need to be translated in english
3. Line 30 you're calling an empty css file

Oh and by the way please check out issues mentioned by the automatic review : http://ventral.org/pareview/httpgitdrupalorgsandboxsdramos1737204git

sdramos’s picture

sdramos commented

Thanks Quentin Albrand, I have corrected the error messages on coding standards, I have been guided http://ventral.org/pareview/httpgitdrupalorgsandboxsdramos1737204git. Your help was a great contribution.

ColonelForbinX’s picture

ColonelForbinX commented

Your module name is drag_drop_block, but your folder name is drag_and_drop_block (at least when I cloned it from your sandbox's git clone link).

Throughout drag_drop_block.module there are several places where you've broken a string up into multiple lines for readability, such as your SQL queries. In many of these case there is hanging whitespace, or inconsistent indentation on the multiple lines. It's generally best to only indent any line with 2 spaces unless there is a specific reason for doing otherwise.

Maybe I'm missing something, but the entire function drag_drop_block_loadconfig() seems wasteful. The inline javascript definitely needs to be moved to a .JS file for maintainability. It seems to me that it isn't necessary to query the database for region names on the server-side at all since you know that each region has a class of ".region", and that each region name begins with ".region-". By removing drag_drop_block_loadconfig() and moving the javascript to its own file, you can:

  • Remove an unnecessary database query that is being performed on every page load
  • Give Drupal the ability to cache your javascript with other modules' code.
  • Only output your javascript code once, instead of once for every region on the page.
  • Give automated code reviewers access to your JS code styling, which also has some whitespace/indentation irregularities that are not being detected because they are hidden in a string.
  • Make it MUCH easier to change/maintain your javascript in the future.

drag_drop_block_save_order() could be commented/documented so it is more apparent what you are doing.

You should also use a more descriptive URL for your AJAX callback in hook_menu(). Maybe something like "/drag-drop-blocks/ajax/save" instead of just "/save-order-block", which could cause conflicts.

Just a suggestion -- make it so some kind of "save" button needs to be hit before block regions/ordering is saved (like on the block page). Either that, or have the ability to toggle drag-and-drop mode (then you could highlight moveable blocks as your hovered over them). Right now it is very easy for someone to accidentally drag a block to a different region, and as soon as they let go of the mouse, the change they made is permanent!

Great module! Works as expected.

ColonelForbinX’s picture

ColonelForbinX commented
Status: Needs review » Needs work

Forgot to change status.

mayank-kamothi’s picture

mayank-kamothi commented

Hi,sdramos

I have checked your module and integrate in my system and drag and drop blocks to move between regions but it not save for me.

Mayank kamothi

mayank-kamothi’s picture

mayank-kamothi commented

Hi,sdramos

Manual Review:

I have checked you code and find some space issue in your module file like there are so many place in your module file which have trailing spaces, so remove space from those lines.

Mayank Kamothi

sdramos’s picture

sdramos commented
Title: Drag and Drop Block module » Drag and Drop Block

I use it and works well, can you tell me more detail which theme and which browser you're using or other information.

Thanks mayank-kamothi.

sdramos’s picture

sdramos commented

Thanks ColonelForbinX.
I'm working on that.
In the case, when the block is dragged and saved automatically. This was one of the ideas to develop this module's saved automatically. An alternative may be to display a message layer when the block has moved. thank you very much for your suggestion. It really helped me.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Issue tags: +PAreview: security

This needs some CSRF protection: http://drupalscout.com/tags/csrf

rwinikates’s picture

rwinikates commented

Progress here seems stalled, this looks like a very useful module, what needs to be done to move this forward? I notice that sdramos has implemented many of the changes ColonelForbinX suggested. I do not know enough about CSRF to know if he has addressed greggles suggestion. Happy to test if it has been taken care of.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

If sdramos has done work to protect against csrf he should mark the issue as "needs review" and ideally point to commit messages to say where that change was.

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.

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)