Compact Forms uses ugly inline JavaScript currently. We can do better in Drupal by leveraging Drupal's JS settings.

Attached patch not only implements Drupal.behaviors, but also does a major clean-up of the module's code, especially regarding FAPI and coding standards.

It also removes Windows line-endings in some of the module's files.

CommentFileSizeAuthor
#8 compact_forms-HEAD-settings.patch2.19 KBsun
#7 compact_forms-HEAD.patch6.06 KBsun
#6 compact_forms-404132-2.patch8.12 KBpounard
#5 compact_forms-404132.patch8.11 KBpounard
#3 compact_forms-HEAD.loading.patch3.62 KBsun
compact_forms-HEAD.clean-up.patch17.13 KBsun

Comments

christefano’s picture

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

This is excellent. The patch is a bit hard to read but that's what happens with a major rewrite.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Fixed

Committed with slight optimizations/additions.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Version: 5.x-1.0 » 6.x-1.x-dev
Status: Fixed » Needs review
StatusFileSize
new compact_forms-HEAD.loading.patch3.62 KB

Now let's advance on this. It's completely senseless to load JavaScript settings on pages that do not contain a form that ought to be compacted.

Attached patch implements this.

pounard’s picture

pounard
Primary language French
commented

Also look at patch at #452170: As I promised! Code cleanup, this patch overrides some things I've done, I think merging both should be good.

pounard’s picture

pounard
Primary language French
commented
StatusFileSize
new compact_forms-404132.patch8.11 KB

And I did merged them, see attached patch

pounard’s picture

pounard
Primary language French
commented
StatusFileSize
new compact_forms-404132-2.patch8.12 KB

My error, "user-login" is the form when you get to mydomain/user and "user-login-form" is what you get in user login block!
I reverted this change!

My drupal see it as user-login-block indeed, tired to make useless patch, see last one attached and fix if you want:)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Fixed
StatusFileSize
new compact_forms-HEAD.patch6.06 KB

Hm. Ok. I have extracted the usable parts from your patch. Please note that when speaking of "code clean-up", I'm referring to http://drupal.org/coding-standards of Drupal core and proper usage of Drupal core's APIs.

-  $form['compact_forms_ids'] = array(
+  $form[COMPACT_FORMS_IDS] = array(
...
+define('COMPACT_FORMS_IDS',          'compact_forms_ids');
+define('COMPACT_FORMS_STARS',        'compact_forms_stars');
+define('COMPACT_FORMS_COLONS',       'compact_forms_colons');
+define('COMPACT_FORMS_FIELD_SIZE',   'compact_forms_field_size');
+define('COMPACT_FORMS_DESCRIPTIONS', 'compact_forms_descriptions');

These are needless defines, which are of no value elsewhere. Even if one of them would be of some value, then a define would still be not appropriate. Drupal core is steadily moving away from defines as well (where possible), replacing them with more meaningful string identifiers.

-    'title' => 'Compact Forms',
-    'description' => 'Configure Compact Forms settings.',
-    'page callback' => 'drupal_get_form',
-    'page arguments' => array('compact_forms_admin_form'),
+    'title'            => 'Compact Forms',
+    'description'      => 'Configure Compact Forms settings.',
+    'page callback'    => 'drupal_get_form',

This is a very debatable coding style. Some prefer it, most do not. The reasoning being that if one ever happens to add new array elements, then the entire array needs to be rewritten (bad).

 /**
- * Implementation of hook_form_alter().
- */
+  * Implementation of hook_form_alter().
+  */

That actually violates coding standards.

+    // '0' options exists, so test here was wrong

Sorry, but no - the condition was correct, because we only need to alter the form when certain configuration values are set. I've rewritten the condition now to clarify that.

Committed attached patch to 6.x (and also to 5.x).

Thanks!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Fixed » Needs review
StatusFileSize
new compact_forms-HEAD-settings.patch2.19 KB

One last minor glitch - the configuration settings use wrong form elements for some options where simple checkboxes would be more appropriate.

pounard’s picture

pounard
Primary language French
commented
Status: Needs review » Fixed

I think using constants is a way more consistent because you won't have to lookup why your code isn't working when you mis-typed something, it's old habit. In fact, I use constants every time the same fixed value has to appear more than twice (here, 3 time per value, and they are constants, even if you don't write as constants, they are fixed).

Remove it, keep this code as you prefer.

For the menu, it's still an old habit, sorry :)

The 0 value is inconsistent because of PHP, not because of signification of the option. If you try the !empty($my_value) on a user set value which points to zero, this won't work. PHP and mixed vars comparison is really annoying with this, be carefull when you use 0 value as an option constant. In PHP 0 means "void", and === usage and some other stuff may be really a mess if you give a real signification to 0 (somewhere else than using "real" int values).

I think even for a "void" option, 0 is dangerous, because you won't know if user set it, if your default value has not been set, or even if you are manipulating the right var.

But, thanks for applying! If you need help on some bugfixes/feature, please ask, I will happily apply your coding standards, this module is great :)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Fixed » Needs review

Reverting status.

Note that these are not "my" coding standards, but the standards worked out and defined by the Drupal community for Drupal core.

pounard’s picture

pounard
Primary language French
commented

Quote from Drupal's coding standards:

As displayed above, there should be one space on either side of an equals sign used to assign the return value of a function to a variable. In the case of a block of related assignments, more space may be inserted to promote readability:

$short         = foo($bar);
$long_variable = foo($baz);

Their exemple for array don't do such indent, but they don't say its bad neither.

They don't really talk about constants usage more than they have to be uppercased neither.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Fixed

There was actually more to improve... committed changes to all branches. Will publish new releases now.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.