Currently we have to do stuff like this:

  $form['email_title'] = array(
    '#type' => 'item', 
    '#title' => t('E-mails'),
  );
  $form['email'] = array(
    '#type' => 'vertical_tabs',
  );

...when it would be a lot nicer to be able to do it from within the vertical tabs element itself:

  $form['email'] = array(
    '#type' => 'vertical_tabs',
    '#title' => t('E-mails'),
    '#description' => t('A helpful description for the overall vertical tab element here.'),
  );
CommentFileSizeAuthor
#27 drupal8.vertical-tabs-title.27.patch5.17 KBsun
#24 drupal8.vertical-tabs-title.24.patch5.13 KBsun
#11 drupal8.vertical-tabs-title.11.patch4.93 KBsun
#8 drupal8.vertical-tabs-title.8.patch5.05 KBsun
#6 drupal8.vertical-tabs-title.6.patch5.03 KBsun
#2 1144716-vertical-tabs-theme.patch6.13 KBDave Reid
#1 1144716-vertical-tabs-theme.patch3.93 KBDave Reid
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Active » Needs review
Issue tags: +vertical tabs
FileSize
1144716-vertical-tabs-theme.patch3.93 KB
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
FileSize
1144716-vertical-tabs-theme.patch6.13 KB

Rest of core's vertical tab elements converted.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Issue tags: +Needs usability testing, +Needs markup review

Adding tags for needing usability and markup reviews.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Needs work 
+++ b/includes/form.inc
@@ -3526,12 +3526,66 @@ function form_process_vertical_tabs($element, &$form_state) {
-  $element = $variables['element'];
+  $element = &$variables['element'];

This tweak could use a small code comment.

+++ b/includes/form.inc
@@ -3526,12 +3526,66 @@ function form_process_vertical_tabs($element, &$form_state) {
+
+

Duplicate newline.

+++ b/includes/form.inc
@@ -3526,12 +3526,66 @@ function form_process_vertical_tabs($element, &$form_state) {
+  $element += array(
+    '#title_display' => 'before',
+  );
...
+  // If #title is not set, we don't display any label or required marker.
+  if (!isset($element['#title'])) {
+    $element['#title_display'] = 'none';
+  }

Let's move the additional munging of #title_display right below the initial default code.

+++ b/includes/form.inc
@@ -3526,12 +3526,66 @@ function form_process_vertical_tabs($element, &$form_state) {
+  // Add element's #type and #name as class to aid with JS/CSS selectors.
+  $element['#attributes']['class'][] = 'form-item';
+  $element['#attributes']['class'][] = 'form-type-vertical-tabs';
...
+  $output = '<div' . drupal_attributes($element['#attributes']) . '>' . "\n";
...
+  $prefix = '<div class="vertical-tabs-panes">';
+  $suffix = '</div>';
...
+  $output .= "</div>\n";

One wrapper too much. Let's simply add the 'vertical-tabs-panes' class to the outer wrapper. More than one container doesn't make much sense here.

+++ b/includes/form.inc
@@ -3526,12 +3526,66 @@ function form_process_vertical_tabs($element, &$form_state) {
+  // Add a class for disabled elements to facilitate cross-browser styling.
+  if (!empty($element['#attributes']['disabled'])) {
+    $element['#attributes']['class'][] = 'form-disabled';
+  }

Not sure whether this has any [valid] effect here. Can probably be removed.

+++ b/includes/form.inc
@@ -3526,12 +3526,66 @@ function form_process_vertical_tabs($element, &$form_state) {
+  $prefix = '<div class="vertical-tabs-panes">';
+  $suffix = '</div>';
...
+    case 'attribute':

If there's a meaning to 'attribute', then the only possible meaning I could see would be a 'title' attribute on the wrapping DIV.

+++ b/includes/form.inc
@@ -3526,12 +3526,66 @@ function form_process_vertical_tabs($element, &$form_state) {
+    case 'none':
...
+      // Output no label and no required marker, only the children.

Stale reference to "and no required marker".

Powered by Dreditor.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

This has the "Needs usability testing" tag - what kind of testing does it need exactly?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Assigned: Dave Reid » sun
Category: feature » task
Status: Needs work » Needs review
Issue tags: -Needs usability testing, -Needs markup review
FileSize
drupal8.vertical-tabs-title.6.patch5.03 KB

I just stumbled over this in a contrib module.

I'd suggest to keep this simple and just get the job done.

Status: Needs review » Needs work

The last submitted patch, drupal8.vertical-tabs-title.6.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal8.vertical-tabs-title.8.patch5.05 KB

Fixed that test failure — of course, the #pre_render needs to check whether any tabs are accessible/visible.

This patch should come back green and could be considered ready.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Feedback, anyone?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

#8: drupal8.vertical-tabs-title.8.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
drupal8.vertical-tabs-title.11.patch4.93 KB

Re-rolled against HEAD.

Feedback, anyone?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue tags: +API clean-up
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Anyone?

saltednut’s picture

saltednut
she/her
Primary language English
Location Chicago
CreditAttribution: saltednut commented

#11 applies cleanly to drupal-8.x-dev. I'm not finding any manual errors when viewing forms that contain vertical tabs around the dev site.

If patch is committed it would save ~3 lines for title assertions on vertical tabs.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Was that an RTBC? ;)

saltednut’s picture

saltednut
she/her
Primary language English
Location Chicago
CreditAttribution: saltednut commented
Status: Needs review » Reviewed & tested by the community
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue tags: -vertical tabs, -API clean-up

#11: drupal8.vertical-tabs-title.11.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

#11: drupal8.vertical-tabs-title.11.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue tags: +vertical tabs, +API clean-up

#11: drupal8.vertical-tabs-title.11.patch queued for re-testing.

YesCT’s picture

YesCT CreditAttribution: YesCT commented

Manual testing. Looks find on blocks (admin/structure/block/manage/system/help/configure), account settings (admin/config/people/accounts), filters (admin/config/content/formats/filtered_html)
Content types like articles don't seems to have titles on the vertical tabs to convert. Still rtbc to me.

patch still applies with offsets.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue tags: -vertical tabs, -API clean-up

#11: drupal8.vertical-tabs-title.11.patch queued for re-testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

#11: drupal8.vertical-tabs-title.11.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +vertical tabs, +API clean-up

The last submitted patch, drupal8.vertical-tabs-title.11.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Reviewed & tested by the community
FileSize
drupal8.vertical-tabs-title.24.patch5.13 KB

Re-rolled against HEAD.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: -vertical tabs, -API clean-up

#24: drupal8.vertical-tabs-title.24.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +vertical tabs, +API clean-up

The last submitted patch, drupal8.vertical-tabs-title.24.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Reviewed & tested by the community
FileSize
drupal8.vertical-tabs-title.27.patch5.17 KB

Hah, playing the re-roll monkey — so lucky to break my own patches :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Category: task » feature

This looks like a nice feature/clean-up. We can commit it once we're under thresholds again.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Category: feature » task
Status: Reviewed & tested by the community » Fixed

Hm. Actually I guess catch has tried to re-test this several times so guess he's cool to commit this.

Committed and pushed to 8.x.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Title: Allow vertical tabs elements to have #title and #description values » Change notice: Allow vertical tabs elements to have #title and #description values
Priority: Normal » Critical
Status: Fixed » Active
Issue tags: +Needs change record

Der. Needs a change notice.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: Change notice: Allow vertical tabs elements to have #title and #description values » Allow vertical tabs elements to have #title and #description values
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record

Change notice: http://drupal.org/node/1886118

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