[D7] Better Blocks

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxphenaproxima2237839git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

I did a very quick test and I could not see any major flaws on the code side while I kind of felt its missing comments in some places. On the other hand, on the UI and from usability perspective, there is no easy way to search block on all regions. Search you have on the UI only search for the active region or selected region/tab. Let's say if I want to search for 'powered by drupal' block and it belongs to 'disabled' tab and let say active tab or region is 'content', then there is no easy way to find the block 'powered by drupal' which belongs to some other region (disabled). This can be a issue from the usability perspective when the block number grows bigger. To find any block to switch the region we have to go through all the regions as opposed to the browser search we can use in the traditional block UI that Drupal gives.

I would love to get the feedback on this comment.

I will second prabeen.giri on the search UI note though it sounds like that's in-progress anyway.

The couple of notes I'd add being...

• Since this module overrides what block does (and doesn't replace it outright) add block as a dependency in the .info file (otherwise if you disable block entirely you get nothing.)
• Since the search form doesn't display items nested under other tabs besides the one you have open I thought I had to click enter to get the search to run which pushes you through to the preview screen, canceling form submission when clicking enter/return in that form (since otherwise it's all drag and drop and save for real actions) might help with that.
• Possibly consider moving non-module files into sub-directories just for the sake of sorting things a bit (css, js, images, etc.)
• There tends to be a preference to move code from jQuery's default $(document).ready() to using Drupal.behaviors.mymodule.attach when possible. There's some notes around that here: Managing JavaScript in Drupal 7 - Behaviors.

I did see there was some code in there around adjusting the weights on blocks, have you tried seeing how that plays with blocks being managed using the Context module in the same regions?

In the past there have been other applications where people were asked to remove SASS/Less/non-drupal/etc. files if they were for things that weren't going to be edited/maintained by the end-user.

That said: this is really nice and I'd definitely consider rolling it into new site builds moving forward.

Looks like the behavior with Context is working right (after a quick poking about with it, anyway).

I do get new blocks outside the better blocks interface when adding new blocks (that aren't assigned to a region from the block add/edit screen) until I clear the site cache (after which they show up in the right place).

FYI.. Just from a quick read, this sounds a lot like https://drupal.org/project/node_level_blocks

Sorry, nevermind the comment in #11. It looks like this module provides a better block overview page.

Hi.
Provide Git link please in description.
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/phenaproxima/2237839.git better_blocks

I didn't find any big nor minor error, only one thing to point out:
In module from line 41 there are 2 statements to check $theme_key, one with check_plain() and second with isset(), combining both of them you'll get same result with less code, only one "theme is not good" message is needed, it could be something like:

  // Trust no-one, Agent Mulder.
  $theme_key = check_plain($theme_key);
  // Theme key is already sanitized, so check that it's
  // a real theme.
  $themes = list_themes();
  if (!isset($themes[$theme_key])) {
    drupal_set_message(t('There is no theme called %key.', array('%key' => $theme_key)), 'error');
    return $form;
  }

CodeSniffer says:

FILE: /var/www/drupal-7-pareview/pareview_temp/better_blocks.js
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
69 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------
FILE: /var/www/drupal-7-pareview/pareview_temp/better_blocks.module
--------------------------------------------------------------------------------
FOUND 12 ERROR(S) AFFECTING 10 LINE(S)
--------------------------------------------------------------------------------
7 | ERROR | Whitespace found at end of line
373 | ERROR | Missing function doc comment
375 | ERROR | Whitespace found at end of line
393 | ERROR | Whitespace found at end of line
397 | ERROR | Whitespace found at end of line
401 | ERROR | Missing function doc comment
408 | ERROR | is_integer() is a function name alias, use is_int() instead
408 | ERROR | Calls to inbuilt PHP functions must be lowercase; expected
| | "strpos" but found "strPos"
418 | ERROR | There should be no white space after an opening "("
418 | ERROR | There should be no white space before a closing ")"
420 | ERROR | Whitespace found at end of line
440 | ERROR | Missing function doc comment
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/css/better_blocks.css
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
11 | ERROR | Expected exactly one new line before closing brace of class
| | definition
36 | ERROR | Expected exactly one new line before closing brace of class
| | definition
--------------------------------------------------------------------------------

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.