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.
Comment | File | Size | Author |
---|---|---|---|
#7 | use_render_array.patch | 11.92 KB | Berdir |
#4 | properties_functionality-1161276-4.diff | 11.38 KB | xatoo |
#2 | properties_coding_style-1161276-2.diff | 23.19 KB | xatoo |
properties_minor_ui_and_code_changes.diff | 30.08 KB | xatoo | |
Comments
Comment #1
BerdirLots 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.
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.
Didn't know that it is as simple as that, nice!
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:
And then in the template, use
render($table);
. And similar for the actions.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.
Comment #2
xatoo CreditAttribution: xatoo commentedThank 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.
Comment #3
BerdirCommited the above patch, so you can now make a new diff based on that one.
Comment #4
xatoo CreditAttribution: xatoo commentedHere 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.
Comment #5
xatoo CreditAttribution: xatoo commentedComment #6
BerdirHm, I don't think that needs the #type table, but I could be wrong. Will review your patch soon.
Comment #7
BerdirHere 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...
Comment #8
BerdirBetter title..
Comment #9
podarok#7 Needs reroll