Hey Berdir,

I'd like to propose a patch with the following changes:
- Moved logica from properties-properties-form.tpl.php to a preprocess function.
- The machine name fields of attributes and properties are now by default auto generated out of the label field.
- The message "" in the properties form is now added as empty text. (Only having table headers without content is ugly).
- The search button above the attributes list now generates an AJAX callback instead of the textfield itself.
- Some changes to the code which the coder module suggested in order to conform to the coding standard.

Sorry for putting it all in one patch. If you'd like to remove something from it, please let me know and i'll re-roll.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Status: Needs review » Needs work

Lots of nice stuff in here, nice work. Some feedback below. Thanks for working on this!

For the next time, I really suggest that you split up your patches, mixing real changes with code style fixes results in huge patches that are harder to review.

+++ b/properties.admin.incundefined
@@ -90,6 +90,11 @@ function properties_admin_attributes_list($form, $form_state) {
+  );
+
+  $form['filter']['submit'] = array(
+    '#type' => 'button',

Yes, I tried to make this work automatically while you're typing, similar to autocomplete. But it is currently very weird, so a separate button is more intuitive.

+++ b/properties.admin.incundefined
@@ -278,6 +279,7 @@ function properties_admin_categories_form($form, &$form_state, $category = NULL)
       'exists' => 'properties_category_load',
+      'source' => array('label'),

Didn't know that it is as simple as that, nice!

+++ b/properties.moduleundefined
@@ -177,6 +181,110 @@ function properties_theme() {
+    }
...
+
+  $variables['table'] = theme('table',
+    array(
+      'header' => $header,
+      'rows' => $rows,
+      'attributes' => array(
+        'id' => $table_id,
+        'class' => array('properties-table'),
+      ),
+      'empty' => t('Add properties to this content. Start by adding categories to it.'),
+    ));
+

The whole preprocess function looks great, makes totally sense to put that there. But since we do that, I think we should make use of render arrays, then you can still change something in the template or another preprocess function. So instead of what you have, write something like:

$variables['table'] = array(
  '#header' => header,
  '#rows' => $rows,
  '#attributes' => array(
    'id' => $table_id,
     'class' => array('properties-table'),
  ),
  '#empty' => t('Add properties to this content. Start by adding categories to it.'),
);

And then in the template, use render($table); . And similar for the actions.

+++ b/properties_template/properties_template.admin.incundefined
@@ -98,31 +98,31 @@ function properties_template_admin_templates_form($form, &$form_state, $template
+    '#disabled' => empty($form_state['template']->new),
+    '#machine_name' => array(
+      'exists' => 'properties_template_load',
+    ),

The source key should be added too here, I guess. Also, I've just noticed that the description refers to categories.. copy/paste error (by us) :)

Powered by Dreditor.

xatoo’s picture

Thank you for your feedback! I agree with all your points and will provide you with a patch with all the functional changes. In the meanwhile, here is a patch with the changes in coding style. Would you like me to diff the functional changes to a codebase with or without this patch applied?

> Yes, I tried to make this work automatically while you're typing, similar to autocomplete. But it is currently very weird, so a separate button is more intuitive.
Indeed. Since a button is present I click on it. But at the same time the textfield loses focus and sends an AJAX request. And one way or another that combination sometime results in an error.

Berdir’s picture

Commited the above patch, so you can now make a new diff based on that one.

xatoo’s picture

Here is a patch with the functional changes. I did not implement the form table as a render array. In order to do that, we might have to wait for #991454: Add element #type table (with tableselect and tabledrag support) to land. Also, I could not find any draggable form in core or contrib which is implemented with a renderable array.

xatoo’s picture

Status: Needs work » Needs review
Berdir’s picture

Hm, I don't think that needs the #type table, but I could be wrong. Will review your patch soon.

Berdir’s picture

FileSize
11.92 KB

Here is a slightly updated patch.

- Uses render arrays for the actions and the table, works fine.
- Re-added the wrapper div you removed. Your change replaced the table with table + actions, which causes the actions to show up twice (and three times if you add another category).
- Then, the drag handlers showed up twice and I had to change the table id inside the preprocess function. I am right now not sure why this worked correctly before. So something seems to be wrong...

Berdir’s picture

Title: Proposal for minor UI and code changes » Render properties table in a preprocess function

Better title..

podarok’s picture

Issue summary: View changes
Status: Needs review » Needs work

#7 Needs reroll