This issue has novice tasks. If you are an experienced core developer and have multiple commit mentions, please review novices' work on these tasks rather than doing them yourself. Feedback from experienced contributors is valued.

Problem/Motivation

#1939062: Convert theme_item_list() to Twig added item-list.html.twig but the variables documented in the Twig template docblock are not 100% accurate. Mainly the 'items' component is not documented accurately:

  1. Each item in the item array no longer has a #wrapper_attributes property.
  2. Each item contains two subvalues: 'value' and 'attributes'.

Proposed resolution

Fix the documentation.

Remaining tasks

User interface changes

n/a

API changes

n/a

CommentFileSizeAuthor
#10 d8-updated_doc-2226853-10.patch849 bytesacouch
#7 d8-updated_doc-2226853-7.patch843 bytesfilijonka
#4 d8-updated_doc-2226853-4.patch867 bytesfilijonka
#1 d8-updated_doc-2226853.patch818 bytesfilijonka

Comments

filijonka’s picture

filijonka commented
Assigned: Unassigned » filijonka
Status: Active » Needs review
StatusFileSize
new d8-updated_doc-2226853.patch818 bytes

Status: Needs review » Needs work

The last submitted patch, 1: d8-updated_doc-2226853.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
commented 
+++ b/core/modules/system/templates/item-list.html.twig
@@ -4,9 +4,8 @@
- * - items: A list of renderable items. Each item has a #wrapper_attributes
- *   property which contains any HTML attributes which should be applied to the
- *   <li> tag.
+ * - items: A list of renderable items. Each item holds two subvalues attributes and value
+ *   which is applied within the <li> tag.

Thanks @filijonka. A couple things:

I think we should split out the items and make a sublist to show more obviously that items contains 'attributes' and 'value'. Check node-add-list.html.twig for a somewhat similar example.

The first line of the comment goes over 80 characters, see https://drupal.org/node/1354#drupal.

filijonka’s picture

filijonka commented
StatusFileSize
new d8-updated_doc-2226853-4.patch867 bytes

thanks for the feedback @cottser

filijonka’s picture

filijonka commented
Status: Needs work » Needs review
star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs review » Needs work 
+++ b/core/modules/system/templates/item-list.html.twig
@@ -4,9 +4,9 @@
+ * - items: A list of renderable items. Each item has following properties
+ *   - attributes: HTML attributes to be applied to the list
+ *   - value: The content of the list element

Thanks, looking better! A few things:

'items' is just an array, not a render array. So we shouldn't say it contains renderable items. I suggest something along these lines:

"A list of items. Each item contains:"

Then for the sub-elements, each of those needs to be a complete sentence (ends in a period) per https://drupal.org/node/1354#general. Other than that looks pretty good to me :)

filijonka’s picture

filijonka commented
Status: Needs work » Needs review
StatusFileSize
new d8-updated_doc-2226853-7.patch843 bytes
star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs review » Needs work 
+++ b/core/modules/system/templates/item-list.html.twig
@@ -4,9 +4,9 @@
+ *   - attributes: HTML attributes to be applied to the list.
...
  * - attributes: HTML attributes to be applied to the list.

I don't think we should have the same documentation for these two variables because they are quite different. One is for the ul/ol and one is for the li. I think this is looking good otherwise :)

filijonka’s picture

filijonka commented

ehm that was odd, i thought i wrote listelement on the the first one, would that be alright? i.e HTML attributes to be applied to the listelement

acouch’s picture

acouch commented
StatusFileSize
new d8-updated_doc-2226853-10.patch849 bytes

Attached is updated wording:

+++ b/core/modules/system/templates/item-list.html.twig
@@ -4,9 +4,9 @@
+ *   - attributes: HTML attributes to be applied to each list item.
...
  * - attributes: HTML attributes to be applied to the list.
acouch’s picture

acouch commented
Status: Needs work » Needs review
filijonka’s picture

filijonka commented
Status: Needs review » Reviewed & tested by the community

thanks acouch

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 98b2898 and pushed to 8.x. Thanks!

  • Commit 98b2898 on 8.x by alexpott: 
    Issue #2226853 by filijonka, acouch: Variables documented in item-list....

Status: Fixed » Closed (fixed)

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

filijonka’s picture

filijonka commented
Assigned: filijonka » Unassigned