To make the functionality of expanding the machine readable name, used in content-type forms, more generic and reusable we need an form element.

http://drupal.org/files/issues/content_type_add_interaction.png

http://drupal.org/node/471242

CommentFileSizeAuthor
#6 form_elements_expandable_2_503816.patch5.49 KBskilip
#3 form_elements_expandable_1_503816.patch5.48 KBskilip
#2 form_inc_503816.patch1.36 KBskilip
#1 system_module_503816.patch718 bytesskilip
#1 form_inc_503816.patch1.2 KBskilip
#1 form_js_503816.patch2.33 KBskilip

Comments

skilip’s picture

skilip commented
Title: New form element 'textfield_expandable' » Make form elements expandable
StatusFileSize
new form_js_503816.patch2.33 KB
new form_inc_503816.patch1.2 KB
new system_module_503816.patch718 bytes

Well, an extra form element was a little overdone. All action is done by javascript, so I'll stick to a preprocess function which adds the script (behavior) when #expandable is true.

skilip’s picture

skilip commented
StatusFileSize
new form_inc_503816.patch1.36 KB

Removed _form_set_class()

skilip’s picture

skilip commented
Status: Active » Needs review
Issue tags: +d7uxsprint
StatusFileSize
new form_elements_expandable_1_503816.patch5.48 KB

Added possibility to use selection lists. Rerolled a singe patch for all three files

Status: Needs review » Needs work

The last submitted patch failed testing.

moshe weitzman’s picture

moshe weitzman commented

Nice work. Instead of calling drupal_add_js() directly, please set #attached_js instead. Better for caching.

skilip’s picture

skilip commented
Status: Needs work » Needs review
StatusFileSize
new form_elements_expandable_2_503816.patch5.49 KB

Thanks for reviewing moshe.

moshe weitzman’s picture

moshe weitzman commented

tested and it works for me. i would change the code comment to mention #attached_js instead of drupal_add_js. if you did use drupal_add_js, then add parens at the end so it hyperlinks properly in the api docs.

the js code could probably use a code review. pinging quicksketch now ...

am i supposed to see a link for closing the expanded elements after opening? i am not seeing that with or without the patch.

skilip’s picture

skilip commented

You should see a [done] link after expanding, weird you don't see it. Which browser did you test it on?I'll try to reproduce it.

Are you saying that the comments are automatically parsed for API docs? That's awesome. I'll put some more love into it then. I also should also attach expandable functionality for the rest of the form field types. Currently the patch only provedes this for textfields and select lists. Next patch...

Bojhan’s picture

Bojhan commented

Can you put some screens up of the interaction.

skilip’s picture

skilip commented

http://skitch.com/skilip/bi746/block-edit-regions-expandable

Bojhan’s picture

Bojhan commented

Can't it just save, when you select one? I don't see the point in, the done button.

skilip’s picture

skilip commented

@Bojhan: The [done] link is only intended to re-hide the element. Initially I didn't place the link there but that was a bit confusing. Having. The possibility to expand something, brings the expectation it can be collapsed back in it's original state.

Bojhan’s picture

Bojhan commented

Well, I undestand - but I think it should behave as all other form expands we have in Drupal (content types) that, you edit it and it closes automaticly.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Closed (won't fix)

This is somehow related to #471018: Generic pattern/js for hiding machine readable names (applied to menu and content types).

However, I don't see the point in hiding form elements as is and putting a link next to them to unhide them. That usually means that the form should either use vertical tabs or some other better organization (like fieldsets) to conditionally display elements.

For this sake, I'm marking this won't fix. Feel free to re-open this issue if you have good arguments for why this would be a good UI pattern.

skilip’s picture

skilip commented

@tha_sun: this patch was based on the 'human readable / system name' issue and was discussed at the D7UX sprint. I wanted the behavior be really generic, that's why I wrote if for form elements.

Since this is discussed at #471018: Generic pattern/js for hiding machine readable names (applied to menu and content types), there's no need for further follow-ups here. I missed the boat again ;)