Create Documentation For Container Form Elements

Well, it is probably too late in the development cycle to remove the Fieldgroup element, so I guess we are stuck documenting it.

We do the same with '#type' => 'operations' vs '#type' => 'dropbutton'. \Drupal\Core\Render\Element\Operations extends Dropbutton, for only minor differences.

That's not really an answer, sorry, just pointing out it's not the only mostly-useless render element around.

Looks pretty good! A few minor things to clear up:

1. +++ b/core/lib/Drupal/Core/Render/Element/Actions.php
   @@ -16,6 +16,15 @@
     * Use of the 'actions' element as an array key helps to ensure proper styling
     * in themes and to enable other modules to properly alter a form's actions.
   

   This is confusing. There are two things:

   a) Naming the form array key 'actions'

   b) Using #type => 'actions'

   And there are two effects. I think (a) leads to "other modules can alter", and (b) leads to "proper styling". Right?

   So let's separate this into two sentences.

   Also I think it would make sense here and in the other "grouping" elements, to have a statement like:

   To group elements, define the parent element, and put each child element as a nested array inside the parent.

   This can possibly be worded better... I guess the usage examples imply this, but it probably wouldn't hurt to state it?

2. +++ b/core/lib/Drupal/Core/Render/Element/Container.php
   @@ -16,6 +16,22 @@
   + * Usage example:
   + * @code
   + * $form['accommodation'] = array(
   + *   '#type' => 'container',
   + *   '#attributes' => array(
   + *     'class' => 'accomodation',
   

   Would someone normally use Container itself, or one of the other container elements (fieldset, details, etc.)?

   Maybe above this, we should put the word "generic" in the docs somewhere, and note that normally you would use one of the more specific container elements? Not sure...

3. +++ b/core/lib/Drupal/Core/Render/Element/Details.php
   @@ -15,6 +15,26 @@
   + * - #title: The title of the details container. Defaults to "Details"
   + * - #open: Indicates whether the contained elements should be visible.
   + *    Defaults to FALSE
   

   Nitpick: Both of these bullet items should end in .

4. +++ b/core/lib/Drupal/Core/Render/Element/Details.php
   @@ -15,6 +15,26 @@
   + * - #open: Indicates whether the contained elements should be visible.
   + *    Defaults to FALSE
   

   Also do you think "should be visible" should maybe say something like "should start out as visible" or something like that? Because when I see "should be visible" when the property is "#open"... it makes me a bit confused.

5. +++ b/core/lib/Drupal/Core/Render/Element/Details.php
   @@ -15,6 +15,26 @@
   + *   '#submit' => array('::submitCacheClear'),
   

   Will this work, or does it need to be array($this, '::submitCacheClear') instead of just '::submitCacheClear' ?

6. +++ b/core/lib/Drupal/Core/Render/Element/Fieldgroup.php
   @@ -13,6 +13,24 @@
     * In default rendering, the only difference between a 'fieldgroup' and a
     * 'fieldset' is the CSS class applied to the containing HTML element.
   

   Should we add a note here saying "Normally, use a ... "?

7. +++ b/core/lib/Drupal/Core/Render/Element/Fieldgroup.php
   @@ -13,6 +13,24 @@
     *
   + * @code
   

   Missing the "Usage example:" heading.

8. +++ b/core/lib/Drupal/Core/Render/Element/Fieldgroup.php
   @@ -13,6 +13,24 @@
   + * $form['name'] = array(
   + *   '#type' => 'fieldgroup',
   + *   '#title' => t('View basic information'),
   

   I was very confused about why the array key here was 'name'?

9. +++ b/core/lib/Drupal/Core/Render/Element/Fieldgroup.php
   @@ -13,6 +13,24 @@
   + * ...
   

   We don't have ... in any of the other examples. Probably shouldn't have it here either?

10. +++ b/core/lib/Drupal/Core/Render/Element/Fieldset.php
    @@ -15,6 +15,24 @@
      * In default rendering, the only difference between a 'fieldgroup' and a
      * 'fieldset' is the CSS class applied to the containing HTML element.
    

    Should we add a note here saying "Normally, use a ... "?

11. +++ b/core/lib/Drupal/Core/Render/Element/Fieldset.php
    @@ -15,6 +15,24 @@
    + * @code
    

    See review notes on the Fieldgroup... Also yes I think we should only have the example once. So whichever one is the silly one, let's put "Normally, use a .... See ... for documentation and usage." in there, and then just have the docs in the normally used one.

12. +++ b/core/lib/Drupal/Core/Render/Element/Table.php
    @@ -17,6 +17,39 @@
    + * - #rows: A two dimensional array containing the rows and columns of the table
    + *   to be displayed.  Alternatively specify the data for the table as child
    

    Here I think it would be better to say:

    An array of the rows to be displayed. Within each row, the cells are ....

    Because I think this can actually be fairly complicated... See
    https://api.drupal.org/api/drupal/core!modules!system!templates!table.ht...

    Probably we should link to this documentation?

    Also, nitpick: should only be one space after .

13. +++ b/core/lib/Drupal/Core/Render/Element/Table.php
    @@ -17,6 +17,39 @@
    + *   to be displayed.  Alternatively specify the data for the table as child
    + *   elements of the table element.
    

    If they are specifying it as child elements, in what order (rows, then within that columns, or vice versa)?

14. +++ b/core/lib/Drupal/Core/Render/Element/Table.php
    @@ -17,6 +17,39 @@
    + *   providing responsive tables.  Defaults to TRUE.
    

    Nitpick: Two spaces after . should only be 1

15. +++ b/core/lib/Drupal/Core/Render/Element/VerticalTabs.php
    @@ -13,8 +13,41 @@
    + * Poperties:
    

    Typo: Properties

16. +++ b/core/lib/Drupal/Core/Render/Element/VerticalTabs.php
    @@ -13,8 +13,41 @@
    + * - #default_tab: The id of the details element to be used as the default tab
    

    Nitpick: id => ID

    Also this should end in .

    And ...needs to tell how someone would know what the ID is. It is not obvious to me. It seems like, from the example, that it is the HTML ID attribute of the tab, but how would someone know what that ID would be? Doesn't it depend on the theme? Ugh.

That looks perfect to me, aside from a bit of awkward grammar. :) How about:

Use of a single Actions element with an array key of 'actions' to group the
primary submit buttons on a form helps to ensure proper styling
in themes, and enables other modules to properly alter a form's actions.
[slight change to last line]

Looks really good!

A few very minor things to fix and I think we're good to go:

1. +++ b/core/lib/Drupal/Core/Render/Element/Container.php
   @@ -16,6 +16,31 @@
   + * $form['needs_accomodation'] = array(
   ...
   + *   '#title' => 'Need Special Accomodations?',
   ...
   + *     'class' => 'accomodation',
   ...
   + *       'input[name="needs_accomodation"]' => array('checked' => FALSE),
   

   spelling error: should be accommodation

2. +++ b/core/lib/Drupal/Core/Render/Element/Details.php
   @@ -13,7 +13,26 @@
   + * - #open: Indicates whether the container should be open by default.
   + *    Defaults to FALSE.
   

   nitpick: One space too many indentation in 2nd line here.

3. +++ b/core/lib/Drupal/Core/Render/Element/Details.php
   @@ -13,7 +13,26 @@
   + * Usage example:
   + * @code
   + * $form['author'] = array(
   + *   '#type' => 'details',
   + *   '#title' => 'Author',
   + * );
   + *
   + * $form['author']['name'] = array(
   + *   '#type' => 'textfield',
   + *   '#title' => t('Name'),
   + * );
   + * @endcode
     *
   

   Hm. So the details element docs above say it can be used outside of forms, but this example contains a form field. I guess that is OK, but maybe it would make sense to create an example that just has some markup in it rather than a form field?

   ...

   OK I got to the end of the patch and came back here, because I found out that you might need to use details in a form for a vertical tabs element. So I guess the example can stay as it is.

   I think it would be good to have an @see to the vertical tabs element here though?

4. +++ b/core/lib/Drupal/Core/Render/Element/Fieldgroup.php
   @@ -11,7 +11,12 @@
   + * @endcode
   

   This line should be removed, and I suppose also the blank line above it. Got left in by mistake in the last revision I guess. :)

5. +++ b/core/lib/Drupal/Core/Render/Element/Table.php
   @@ -17,6 +17,41 @@
   + * - #empty: Text to display when no rows are present. Alternatively specify
   + *   the data for the table as child elements of the table element. Table
   + *   elements would contain rows elements that would in turn contain
   + *   column elements.
   

   Doesn't this "Alternatively" part go with #rows rather than #empty?

6. +++ b/core/lib/Drupal/Core/Render/Element/VerticalTabs.php
   @@ -13,8 +13,42 @@
   + * - #default_tab: The html ID of the rendered details element to be used as
   

   html -> HTML

Almost there! Not quite everything from the last review got fixed:

1. +++ b/core/lib/Drupal/Core/Render/Element/Container.php
   @@ -16,6 +16,31 @@
   + * $form['needs_accomodation'] = array(
   

   I think we should also correct the spelling in the array keys and classes in this section

   accomodation -> accommodation

2. +++ b/core/lib/Drupal/Core/Render/Element/Fieldgroup.php
   @@ -11,7 +11,12 @@
   + * @endcode
   

   This still needs to be removed.

Still two one-m misspellings:

+ *     'class' => 'accomodation',
+ *   ),
+ *   '#states' => array(
+ *     'invisible' => array(
+ *       'input[name="needs_accomodation"]' => array('checked' => FALSE),

+++ b/core/lib/Drupal/Core/Render/Element/Table.php
@@ -17,6 +17,41 @@
+ *   of cell contents or an array of properties as described in table.html.twig

This sentence doesn't end in a . but ... not sure API module would turn table.html.twig into a link if it were... probably OK as-is.

So, this is looking great!

Committed 8b2e3f0 and pushed to 8.0.x. Thanks!

Woot! Looks like the parent issue just has 2 more children: "hidden" things and "button" things.