Currently we have to do stuff like this:

<?php
  $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:

<?php
  $form
['email'] = array(
   
'#type' => 'vertical_tabs',
   
'#title' => t('E-mails'),
   
'#description' => t('A helpful description for the overall vertical tab element here.'),
  );
?>
Files: 
CommentFileSizeAuthor
#27 drupal8.vertical-tabs-title.27.patch5.17 KBsun
PASSED: [[SimpleTest]]: [MySQL] 49,580 pass(es).
[ View ]
#24 drupal8.vertical-tabs-title.24.patch5.13 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.vertical-tabs-title.24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 drupal8.vertical-tabs-title.11.patch4.93 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.vertical-tabs-title.11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 drupal8.vertical-tabs-title.8.patch5.05 KBsun
PASSED: [[SimpleTest]]: [MySQL] 48,761 pass(es).
[ View ]
#6 drupal8.vertical-tabs-title.6.patch5.03 KBsun
FAILED: [[SimpleTest]]: [MySQL] 47,994 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#2 1144716-vertical-tabs-theme.patch6.13 KBDave Reid
PASSED: [[SimpleTest]]: [MySQL] 29,461 pass(es).
[ View ]
#1 1144716-vertical-tabs-theme.patch3.93 KBDave Reid
PASSED: [[SimpleTest]]: [MySQL] 29,467 pass(es).
[ View ]

Comments

Dave Reid’s picture

Status:Active» Needs review
Issue tags:+vertical tabs
StatusFileSize
new3.93 KB
PASSED: [[SimpleTest]]: [MySQL] 29,467 pass(es).
[ View ]
Dave Reid’s picture

StatusFileSize
new6.13 KB
PASSED: [[SimpleTest]]: [MySQL] 29,461 pass(es).
[ View ]

Rest of core's vertical tab elements converted.

Dave Reid’s picture

Adding tags for needing usability and markup reviews.

sun’s picture

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

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

sun’s picture

Assigned:Dave Reid» sun
Category:feature» task
Status:Needs work» Needs review
Issue tags:-Needs usability testing, -Needs markup review
StatusFileSize
new5.03 KB
FAILED: [[SimpleTest]]: [MySQL] 47,994 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new5.05 KB
PASSED: [[SimpleTest]]: [MySQL] 48,761 pass(es).
[ View ]

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

Feedback, anyone?

sun’s picture

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

sun’s picture

StatusFileSize
new4.93 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.vertical-tabs-title.11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-rolled against HEAD.

Feedback, anyone?

sun’s picture

Issue tags:+API clean-up
sun’s picture

Anyone?

brantwynn’s picture

#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

Was that an RTBC? ;)

brantwynn’s picture

Status:Needs review» Reviewed & tested by the community
sun’s picture

Issue tags:-vertical tabs, -API clean-up

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

sun’s picture

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

sun’s picture

Issue tags:+vertical tabs, +API clean-up

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

YesCT’s picture

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

Issue tags:-vertical tabs, -API clean-up

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

catch’s picture

#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

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new5.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.vertical-tabs-title.24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-rolled against HEAD.

catch’s picture

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

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new5.17 KB
PASSED: [[SimpleTest]]: [MySQL] 49,580 pass(es).
[ View ]

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

webchick’s picture

Category:task» feature

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

webchick’s picture

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

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

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

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