Proposed commit message

Issue #1842140 by mdrummond, tonystar, gavin.hughes, killerpoke, lauriii, droweski, stevector, omg-its-maggie, jenlampton, Renee S: Remove title and wrapper div from theme_item_list.

Problem/Motivation

We are currently printing a title and wrapper div in item list templates. Templates really should be as concise and semantic as possible, and only print a div when it's really needed, the title should be a separate element. The class from the wrapper div can be moved to the ul/ol tag, and the CSS can be updated to match.

Proposed resolution

When we need a heading tag and/or a wrapper div, we should call theme_links instead. That theme function can then call theme('item_list') which will generate a nice clean list. When we only need a nice clean list, we can call this function directly.

Proposed code fo the item list template:

{% spaceless %}
  {%- if list_type == 'ul' -%}
    <ul{{ attributes }}>
  {%- else -%}
    <ol{{ attributes }}>
  {%- endif %}
    {%- for item in items -%}
      <li{{ item['#wrapper_attributes'] }}>{{ item }}</li>
    {%- endfor -%}
  {%- if list_type == 'ul' -%}
    </ul>
  {%- else -%}
    </ol>
  {%- endif %}
{% endspaceless %}
Location of item_list call Changes (this patch)
core/authorize.php N/A
core/modules/field/lib/Drupal/field/Plugin/views/field/Field.php N/A
core/includes/theme.maintenance.inc N/A
core/modules/node/node.module N/A
core/modules/system/css/system.theme.css CSS cleanup
core/modules/system/lib/Drupal/system/Tests/Theme/FunctionsTest.php update tests to exclude title
core/modules/system/system.module Update CSS - classes moved to ul
core/modules/toolbar/tests/modules/toolbar_test/toolbar_test.module N/A
core/modules/tour/lib/Drupal/tour/TourRenderController.php Update CSS - id and class moved to UL
core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php N/A
core/modules/views/lib/Drupal/views/Plugin/views/field/PrerenderList.php N/A
core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.php Update CSS - classes moved to ul
core/modules/views/views.theme.inc Update CSS - classes moved to ul
core/update.php N/A

Remaining tasks

  • Remove the title from theme_links
  • Move the class from the wrapper div onto the ul itself, and delete the wrapper div
  • Update all CSS to reflect the new location of the class
Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Reroll the patch if it no longer applies. Instructions
Update the issue summary Instructions
Update the issue summary noting if allowed during the beta Instructions
Embed before and after screenshots in the issue summary Novice Instructions

User interface changes

None.

API changes

Call theme('links') when you need a wrapper div and/or a title.
Call theme(item_list) when you only need a list.

Related

#311011: Replace links.html.twig with item-list--links.html.twig
#2032645: Replace calls to theme('item_list') with calls to theme('links') for links, when a heading or wrapper div is warranted
#891112: Replace theme_item_list()'s 'data' items with render elements
#1777326: Replace theme_links() with theme_item_list()
#1910996: Update theme_item_list to call theme_ol or theme_ul
#1980004: [meta] Creating Dream Markup

Files: 
CommentFileSizeAuthor
#132 remove_title_and-1842140-132.patch20.9 KBManjit.Singh
#128 remove_title_and-1842140-128.patch20.88 KBrteijeiro
#123 remove_title_and-1842140-123.patch21.94 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,439 pass(es), 13 fail(s), and 0 exception(s). View
#117 remove-title-wrapper-div-theme_item_list-1842140-117.patch20.93 KBakalata
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,303 pass(es), 13 fail(s), and 0 exception(s). View
#109 interdiff-107-109.txt1.35 KBBarisW
#109 remove-title-wrapper-div-theme_item_list-1842140-109.patch20.6 KBBarisW
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,451 pass(es). View
#100 remove-title-wrapper-div-theme_item_list-1842140-100.patch20.23 KBManuel Garcia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,980 pass(es). View
#94 remove-title-wrapper-div-theme_item_list-1842140-63.patch17.28 KBvks7056
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,918 pass(es), 12 fail(s), and 0 exception(s). View
#93 remove-title-wrapper-div-theme_item_list-1842140-63.patch0 bytesvks7056
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,929 pass(es). View
#84 interdiff-1842140-52-2351023-3.txt461 bytesandrewsuth
#83 remove-title-wrapper-div-theme_item_list-1842140-62.patch20.07 KBandrewsuth
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,033 pass(es). View
#62 1842140-52.patch19.62 KBtonystar
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,923 pass(es). View
#17 drupal-clean_up_item_list-1842140-17.patch10.47 KBstevector
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-clean_up_item_list-1842140-17.patch. Unable to apply patch. See the log in the details link for more information. View
#13 drupal-clean_up_item_list-1842140-13.patch10.45 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-clean_up_item_list-1842140-13.patch. Unable to apply patch. See the log in the details link for more information. View
#2 ulol_twig_1842140.patch1.41 KBRenee S
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch ulol_twig_1842140.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

Renee S’s picture

Assigned: Unassigned » Renee S

[edit: nevermind, heard access isn't being given anymore. will submit teeny patch :) ]

Renee S’s picture

FileSize
1.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch ulol_twig_1842140.patch. Unable to apply patch. See the log in the details link for more information. View

Teeny tiny patch attached (I'm assuming this is for stark, yah? :)

Renee S’s picture

Status: Active » Needs review
killerpoke’s picture

Assigned: Renee S » killerpoke
killerpoke’s picture

Assigned: killerpoke » Unassigned
Issue tags: +drupal-at-twig

looks good to me, but I'm not surre about the indention.

c4rl’s picture

Status: Needs review » Closed (works as designed)

I'm closing this since at this point we just need to get these converted to core, then refactor later. This is a core issue, not a sandbox issue.

c4rl’s picture

Title: remove title and wrapper div from item lists » Remove title and wrapper div from theme_item_list
Project: » Drupal core
Version: » 8.x-dev
Component: Markup or variable cleanup (front-end branch) » theme system
Status: Closed (works as designed) » Active
Issue tags: -drupal-at-twig +theme system cleanup

Nevermind, I'm just going to move this into the core queue and revise the summary. Less duplication :) Will revise summary.

Related: #1910996: Update theme_item_list to call theme_ol or theme_ul

c4rl’s picture

Issue summary: View changes

add templates affected

c4rl’s picture

Just a follow-up for the Twig sandbox folks, I'm removing ol.html.twig and ul.html.twig per #1910996: Update theme_item_list to call theme_ol or theme_ul, we'll follow-up there.

jenlampton’s picture

Issue tags: +markup

tagging

jenlampton’s picture

Issue tags: +dreammarkup

tagging

falcon03’s picture

The title support should not be removed. It's there for accessibility: menus, in fact, are lists; and each menu needs a title.

falcon03’s picture

Issue summary: View changes

Better summary

jenlampton’s picture

Issue summary: View changes

updated issue summary

jenlampton’s picture

Issue summary: View changes

related

jenlampton’s picture

@falcon03 if that's the case then menus should not be calling theme_item_list(), they should be calling theme_links() which has a wrapper div and title, or better, theme_menu(). See #311011: Replace links.html.twig with item-list--links.html.twig and #1834022: Combine theme_menu_tree and theme_menu_link, and rename theme_menu

edit: Also, since this is just a render array you can always add a #prefix to your render element containing the title if there's no better way to do it. :)

previous:

  $form['start'][$module . '_updates'] = array(
    '#theme' => 'item_list',
    '#items' => $update['pending'],
    '#title' => $module . ' module',
  );

now:

  $form['start'][$module . '_updates'] = array(
    '#theme' => 'item_list',
    '#items' => $update['pending'],
    '#prefix' => '<h4>' . $module . ' module' . '</h4>',
  );

Marking as postponed, this issue is blocked by #2032645: Replace calls to theme('item_list') with calls to theme('links') for links, when a heading or wrapper div is warranted.

jenlampton’s picture

Status: Active » Postponed
FileSize
10.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-clean_up_item_list-1842140-13.patch. Unable to apply patch. See the log in the details link for more information. View

here's a go at cleaning up all the css - item_list is still a theme function, so this may need a reroll if it becomes a twig template soon.

dpintats’s picture

Assigned: Unassigned » dpintats
Status: Postponed » Needs work

Working on this alongside johnnydarkko, who is working on #311011: Replace links.html.twig with item-list--links.html.twig.

dpintats’s picture

Issue summary: View changes

code up

jenlampton’s picture

Issue summary: View changes

list of changes

jenlampton’s picture

Assigned: dpintats » Unassigned
Status: Needs work » Needs review

unassigning.

Status: Needs review » Needs work

The last submitted patch, drupal-clean_up_item_list-1842140-13.patch, failed testing.

stevector’s picture

Status: Needs work » Needs review
FileSize
10.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-clean_up_item_list-1842140-17.patch. Unable to apply patch. See the log in the details link for more information. View

Here's a reroll of #13 just to get it applying again.

Status: Needs review » Needs work
Issue tags: -markup, -theme system cleanup, -dreammarkup

The last submitted patch, drupal-clean_up_item_list-1842140-17.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

up

jibran’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +markup, +theme system cleanup, +dreammarkup

The last submitted patch, drupal-clean_up_item_list-1842140-17.patch, failed testing.

Fabianx’s picture

Issue tags: +Needs reroll

Tagging for re-roll

gavin.hughes’s picture

FileSize
10.47 KB
FAILED: [[SimpleTest]]: [MySQL] 58,626 pass(es), 45 fail(s), and 62 exception(s). View

Rerolled #17 :)

gavin.hughes’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
10.47 KB
FAILED: [[SimpleTest]]: [MySQL] 58,700 pass(es), 45 fail(s), and 62 exception(s). View

Updating status

The last submitted patch, drupal-clean_up_item_list-1842140-22.patch, failed testing.

gavin.hughes’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-clean_up_item_list-1842140-22.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Add dream markup meta to related

droweski’s picture

Patch 22 will not apply to current code base.
Can apply at commit 5fccca655e90d9e7a557fda8d16dc2afab93ac08
I'm currently re-rolling patch against current.

Simpletest test failures look related to pager errors.

droweski’s picture

Issue summary: View changes
FileSize
10.71 KB
FAILED: [[SimpleTest]]: [MySQL] 60,309 pass(es), 47 fail(s), and 62 exception(s). View

Re-rolled patch against current code.
conflict was in core/themes/seven/styles.css
Couldn't get my interdiff to work.
I expect patch will fail testing but now we are at least up to date.

droweski’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 28: drupal-clean_up_item_list-1842140-28.patch, failed testing.

YesCT’s picture

mdrummond’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
11.47 KB
FAILED: [[SimpleTest]]: [MySQL] 62,997 pass(es), 54 fail(s), and 68 exception(s). View

Here's a reroll from #28.

Status: Needs review » Needs work

The last submitted patch, 32: clean-up-item-list-1842140-32.patch, failed testing.

mdrummond’s picture

Status: Needs work » Needs review

I ran the render test that failed on my local dev with the patch applied, and the test passed. So, setting testbot to rerun this.

mdrummond’s picture

Status: Needs review » Needs work

Errr, sorry, wrong issue.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
11.85 KB
FAILED: [[SimpleTest]]: [MySQL] 64,390 pass(es), 17 fail(s), and 6 exception(s). View

Here's a reroll.

I'm not fully grokking the preprocess function: it seems to me like this will probably out the item-list class on list elements rather than ul or ol. But maybe I'm wrong.

Hopefully this is a better starting point to finish patching this up.

Status: Needs review » Needs work

The last submitted patch, 36: clean-up-item-list-1842140-36.patch, failed testing.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
10.94 KB
FAILED: [[SimpleTest]]: [MySQL] 64,209 pass(es), 53 fail(s), and 68 exception(s). View
2.3 KB

New attempt to set the item-list class on the list. This is probably wrong: we'll see if it's more or less wrong.

Also, we may want to change the scope of this issue. The header on the list is already optional in the item-list twig file. This patch just focuses on removing the wrapper div now. Probably the way to go.

Status: Needs review » Needs work

The last submitted patch, 38: clean-up-item-list-1842140-38.patch, failed testing.

joelpittet’s picture

  1. +++ b/core/includes/theme.inc
    @@ -1769,11 +1769,14 @@ function theme_mark($variables) {
    +  $variables['attributes'] = (array) $variables['attributes'];
    

    This line is not needed at all, attributes are magic arrays to start and get converted to Attribute objects during drupal_render.

  2. +++ b/core/modules/simpletest/css/simpletest.module.css
    @@ -35,7 +35,7 @@ table#simpletest-form-table tr.simpletest-group label {
    +div.message > ul.item-list {
    

    This would be nicer if it didn't have the element selector on it, can we say that all direct children with that class get the same style?

  3. +++ b/core/modules/system/css/system.theme.css
    @@ -149,11 +146,11 @@ abbr.form-required, abbr.tabledrag-changed, abbr.ajax-changed {
    +ul.pager {
    ...
    +ul.pager li {
    

    probably want to do .pager.item-list

  4. +++ b/core/modules/views/templates/views-view-summary.html.twig
    @@ -19,14 +19,12 @@
    -  <ul class="views-summary">
    -  {% for row in rows %}
    -    <li><a href="{{ row.url }}"{{ row.attributes }}>{{ row.link }}</a>
    ...
    +<ul class="views-summary item-list">
    +{% for id, row in rows %}
    +  <li><a href="{{ row.url }}"{{ row_classes[id] }}>{{ row.link }}</a>
    

    This row_classes stuff is not right at all... should be row.attributes.

  5. +++ b/core/themes/seven/style.css
    @@ -1,15 +1,123 @@
    +body {
    +  color: #000;
    +  background: #fff;
    +  font: normal 81.3%/1.538em "Lucida Grande", "Lucida Sans Unicode", sans-serif;
    +}
    +a,
    +.link {
    +  color: #0074bd;
    +  text-decoration: none;
    +}
    +a:hover,
    +.link:hover {
    +  text-decoration: underline;
    +}
    +hr {
    +  margin: 0;
    +  padding: 0;
    +  border: none;
    +  height: 1px;
    +  background: #cccccc;
    +}
    +summary,
    +legend {
    +  font-weight: bold;
    +  text-transform: uppercase;
    +}
    +h1,
    +h2,
    +h3,
    +h4,
    +h5,
    +h6 {
    +  font-weight: bold;
    +  margin: 10px 0;
    +}
    +h1 {
    +  font-size: 1.538em;
    +}
    +h2 {
    +  font-size: 1.385em;
    +}
    +h3 {
    +  font-size: 1.231em;
    +}
    +h4 {
    +  font-size: 1.154em;
    +}
    +h5,
    +h6 {
    +  font-size: 1.077em;
    +}
    +p {
    +  margin: 1em 0;
    +}
    +dl {
    +  margin: 0 0 20px;
    +}
    +dl dd,
    +dl dl {
    +  margin-left: 20px; /* LTR */
    +  margin-bottom: 10px;
    +}
    +[dir="rtl"] dl dd,
    +[dir="rtl"] dl dl {
    +  margin-right: 20px;
    +}
    +blockquote {
    +  margin: 1em 40px;
    +}
    +address {
    +  font-style: italic;
    +}
    +u,
    +ins {
    +  text-decoration: underline;
    +}
    +s,
    +strike,
    +del {
    +  text-decoration: line-through;
    +}
    +big {
    +  font-size: larger;
    +}
    +small {
    +  font-size: smaller;
    +}
    +sub {
    +  vertical-align: sub;
    +  font-size: smaller;
    +  line-height: normal;
    +}
    +sup {
    +  vertical-align: super;
    +  font-size: smaller;
    +  line-height: normal;
    +}
    +nobr {
    +  white-space: nowrap;
    +}
    +abbr,
    +acronym {
    +  border-bottom: dotted 1px;
    +}
    +.pager li {
    +  padding: 0.5em;
    +}
    

    none of this should be in here.

mdrummond’s picture

FileSize
9.54 KB
FAILED: [[SimpleTest]]: [MySQL] 64,412 pass(es), 53 fail(s), and 68 exception(s). View
3.64 KB

Here's a patch that addresses the issues in #41.

mdrummond’s picture

Status: Needs work » Needs review
joelpittet’s picture

+++ b/core/modules/views/templates/views-view-summary.html.twig
@@ -19,14 +19,12 @@
-<div class="item-list">
-  <ul class="views-summary">
-  {% for row in rows %}
-    <li><a href="{{ row.url }}"{{ row.attributes }}>{{ row.link }}</a>
-      {% if options.count %}
-        ({{ row.count }})
-      {% endif %}
-    </li>
-  {% endfor %}
-  </ul>
-</div>
+<ul class="views-summary item-list">
+{% for id, row in rows %}
+  <li><a href="{{ row.url }}"{{ row.atributes[id] }}>{{ row.link }}</a>
+    {% if options.count %}
+      ({{ row.count }})
+    {% endif %}
+  </li>
+{% endfor %}
+</ul>

not quite, no need for the [id] in the loop. That code should not change.

mdrummond’s picture

FileSize
9.53 KB
FAILED: [[SimpleTest]]: [MySQL] 64,360 pass(es), 53 fail(s), and 68 exception(s). View
589 bytes

Fixing an error with the attributes.

Status: Needs review » Needs work

The last submitted patch, 44: clean-up-item-list-1842140-44.patch, failed testing.

The last submitted patch, 44: clean-up-item-list-1842140-44.patch, failed testing.

The last submitted patch, 41: clean-up-item-list-1842140-41.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
10.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,742 pass(es), 19 fail(s), and 24 exception(s). View

Moving this forward. Someone could take a look for the visual changes

killerpoke’s picture

Issue tags: +Amsterdam2014
FileSize
10.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,742 pass(es), 19 fail(s), and 24 exception(s). View

I just review patch 1842140-48.patch. I rendered a test list and it looks the same, with and without the patch.

It looks good to me except, one:
You forgot to include changes to core/themes/seven/css/components/table.css

I created a patch that also includes table.css

I think, the only case, there is a side-effect, is when .item-list has a fixed width and a box-size model, that doesn't count padding to it's size. After applying the patch, .item-list element will be bigger then it's original size.

The last submitted patch, 48: 1842140-48.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 49: 1842140-49.patch, failed testing.

tonystar’s picture

Status: Needs work » Needs review
FileSize
16.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,756 pass(es), 9 fail(s), and 69 exception(s). View

Since the "ul" tag has already straightforward meaning we actually don't need special ".item-list" class here. For menu purposes we still have ".menu" class there.

Finally we found all the occurrences of ".item-list" in the Drupal core and got rid of them.

Title is still in the templates for compatibility reasons. Anyway it could be easily overridden in templates or even cut in the future.

All of this was done in agreement with mortendk and johnalbin.

Attached patch also fixes views markup.

tonystar’s picture

And let's not forget to add omg-its-maggie in the commit message!

She helped me a lot with testing, support and inspiration!

Status: Needs review » Needs work

The last submitted patch, 52: 1842140-50.patch, failed testing.

Status: Needs work » Needs review

tonystar queued 52: 1842140-50.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 52: 1842140-50.patch, failed testing.

Status: Needs work » Needs review

tonystar queued 52: 1842140-50.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 52: 1842140-50.patch, failed testing.

tonystar’s picture

FileSize
19.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,764 pass(es), 6 fail(s), and 6 exception(s). View
5.19 KB

Fixed issues related to view_list template and tests.

tonystar’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 59: 1842140-51.patch, failed testing.

tonystar’s picture

Status: Needs work » Needs review
FileSize
19.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,923 pass(es). View

Fixed XPaths in Views Field UI tests.

tonystar’s picture

Assigned: Unassigned » tonystar
tonystar’s picture

Guys, please review!

Two things to note:

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

The patch looks great to me.

The test changes make total sense and tests pass.

A DivKiller issue, which is a nice cleanup overall.

=> RTBC

Fabianx’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 7939567 and pushed to 8.0.x. Thanks!

  • alexpott committed 7939567 on 8.0.x
    Issue #1842140 by mdrummond, tonystar, gavin.hughes, killerpoke, lauriii...
mdrummond’s picture

Getting rid of .item-list is a regression with D8's CSS architecture: https://www.drupal.org/node/1887918.

Now CSS is dependent upon a specific HTML element, ul, which isn't good if an ol is needed. If we were getting rid of the wrapper div, we would have been better off applying .item-list to the ul and using that in the selector rather than a ul element.

  1. +++ b/core/modules/system/css/system.theme.css
    @@ -21,24 +21,6 @@ td.active {
    - * Markup generated by theme_item_list().
    - */
    -.item-list .title {
    -  font-weight: bold;
    -}
    -.item-list ul {
    -  margin: 0 0 0.75em 0;
    -  padding: 0;
    -}
    -.item-list ul li {
    -  margin: 0 0 0.25em 1.5em; /* LTR */
    -  padding: 0;
    -}
    -[dir="rtl"] .item-list ul li {
    -  margin: 0 1.5em 0.25em 0;
    -}
    -
    -/**
    

    Nothing replaces these styles that are removed?

  2. +++ b/core/themes/seven/css/components/menus-and-lists.css
    @@ -1,15 +1,7 @@
    -.item-list ul {
    -  list-style-type: disc;
    -  list-style-image: none;
    -  margin: 0.25em 0 0.25em 1.5em; /* LTR */
    -}
    -[dir="rtl"] .item-list ul {
    -  margin: 0.25em 1.5em 0.25em 0;
    -}
    -.item-list ul li,
    +li,
    

    We're removing styles without any replacement here. Also, this is now applying styles to every single list item anywhere rather than to list items designated as part of an item list.

  3. +++ b/core/themes/seven/css/components/menus-and-lists.css
    @@ -18,13 +10,11 @@ ul.menu li {
    -.item-list ul li.collapsed,
    -ul.menu li.collapsed {
    +li.collapsed {
    

    Again, context gone.

  4. +++ b/core/themes/seven/css/components/menus-and-lists.css
    @@ -18,13 +10,11 @@ ul.menu li {
    -.item-list ul li.expanded,
    -ul.menu li.expanded {
    +li.expanded {
    

    Context gone.

  5. +++ b/core/themes/seven/css/components/tables.css
    @@ -93,7 +93,7 @@ th.active > a:focus:after,
    -td .item-list ul {
    +td ul {
    

    Again, this is now targeting all unordered lists inside of a table cell.

We should probably do a follow-up issue to make sure that we get this CSS in line with D8's CSS architecture.

joelpittet’s picture

Assigned: tonystar » Unassigned
Issue tags: +needs screenshots

Hmm this is a bit of a quick change for having such a large scope. This may need reverting, discussion and mostly screenshots of item lists changed.

I completely agree with the idea behind this, but the rigor in testing is missing. Since it's CSS there is no visual regression tests in core so we need eyeballs.

Fabianx’s picture

Well, John Albin agreed, so we should ping him, before rolling back.

We are close to having visual regression testing working in contrib, fwiw.

We can add the ul item-list also back to classy, as core would not have that class anyway.

Ul,ol are lists - always, so that is redundant information.

I think the do not depend on elems is meant in another context, but need John to chime in.

Fabianx’s picture

Assigned: Unassigned » JohnAlbin

Assigning ...

joelpittet’s picture

You are right the class is quite useless but it seems people have given it meaning where there was none before. Using it to distinguish uls used for menus or in the body copy or in pagers vs ones generated by this function.

That "meaning" needs to be undone for this issue to be actually "fixed" which is why the rigor is needed.

Also see #2335003-23: Rename task-list.html.twig to maintenance-task-list.html.twig for a screenshot of the regression.

omg-its-maggie’s picture

Are you saying that it is ok to change the classname as long as it's a more sensible semantic name?

Because then I agree.

lauriii’s picture

Do we have to have item-list wrapper? Cant we use it as a class the way I suggested on #48?

tonystar’s picture

You are right the class is quite useless but it seems people have given it meaning where there was none before. Using it to distinguish uls used for menus or in the body copy or in pagers vs ones generated by this function.

That "meaning" needs to be undone for this issue to be actually "fixed" which is why the rigor is needed.

@joelpittet

Right. That's why we executed something like grep -r 'item-list' throughout the whole code to fix everything.

We made a basic tests after and everything looked fine but we used installed Drupal that's why we missed that one on the installation page.

***

Also see #2335003-23 for a screenshot of the regression.

@joelpittet

It's kind of a "bug" in maintenance-page.css actually:

  .task-list {
    ...
    list-style-type: none;
    ...
  }

This rule should be added also there:

  .task-list li {
    ...
    list-style-type: none;
    ...
  }

***

I see only 2 potential problems which should be double-checked while deeper testing:

  1. Forgetting OL when moving from DIV.item-list to UL — here we can just clone UL rules for OL, easy.
  2. Breaking selectors like ... div > ul since we have no interjacent DIV anymore — this is not so easy to detect, but using such selectors are just wrong initially.

So, I think we do it right way at all: we definitely don't need .item-list class here. What we need — is exploratory testing. And let's wait @johnalbin feedback of course.

Cottser’s picture

Status: Fixed » Needs review

It sounds like in the meantime this should be reverted and then we can get a more thoroughly tested version in.

git revert 7939567

  • alexpott committed ad5024c on 8.0.x
    Revert "Issue #1842140 by mdrummond, tonystar, gavin.hughes, killerpoke...
Cottser’s picture

Assigned: JohnAlbin » Unassigned
Status: Needs review » Needs work

Thank you @alexpott!

tonystar’s picture

Issue tags: +Twig
webchick’s picture

Issue summary: View changes

Adding participants from #2351023: Bullets are showing in installer to the commit credit string in the issue summary, since they jumped in to fix one of the regressions created by this patch.

lauriii’s picture

Issue summary: View changes

@tonystar: The site was visually broken also on installed site wherever .item-list was before that change used e.g. messages.

andrewsuth’s picture

FileSize
20.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,033 pass(es). View

Rerolled and added the regression patch from #2351023: Bullets are showing in installer

andrewsuth’s picture

Added missing interdiff file.

tonystar’s picture

Status: Needs work » Needs review
Cottser’s picture

Title: Remove title and wrapper div from theme_item_list » Remove title and wrapper div from item-list.html.twig

I think we should start adding URLs/steps to the issue summary where this can be tested. For example the search results uses an item_list theme suggestion.

tim.plunkett’s picture

Issue tags: - +Needs screenshots

Fixing tags.

droplet’s picture

+++ b/core/modules/simpletest/css/simpletest.module.css
@@ -35,7 +35,7 @@ table#simpletest-form-table tr.simpletest-group label {
+div.message > ul {

+++ b/core/themes/bartik/css/style.css
@@ -326,11 +326,11 @@ ul.menu li {
-.item-list ul li {
...
+[dir="rtl"] li {

@@ -494,30 +494,30 @@ h1.site-name {
+.region-header #block-user-login ul,
...
+.region-header #block-user-login ul {

these changes (and a lot on this thread) making the CSS less reusable and harder theming. It just making for the Drupal project, not for all custom Drupal projects. when you adding custom features, you also have to adding many CSS to override the widely pre-applied default style.

I'd suggest to limit the scope. adding re-use .class the ul lists...etc

https://smacss.com/book/type-module

Fabianx’s picture

We could try some visual regression testing using SiteEffect.io for this issue.

mdrummond’s picture

This CSS definitely needs to be fixed. There are a ton of style definitions that depend solely upon element chains, which doesn't fit with D8's CSS architecture. Defining all the behavior of every li everywhere is far too broad. Much better to have class selectors that are less likely to cause problems elsewhere. Having ID's in the selectors makes the specificity very difficult to override: it looks like that may have been there previously, though, so that may or may not be in scope to fix that for this particular issue.

ti2m’s picture

As suggested by @fabianx I have tested the patch that got reverted, just to see what I would catch with the visual regression testing approach described in #2229187

I wasn't able to figure out yet if it is being considered ok for the patch to introduce visual changes. Meaning I'm not sure if e.g. the spacing between primary tabs is allowed to change or not. As siteeffect isn't public yet I just uploaded gifs to dropbox to give you an idea. The preview mode actually works quiet nicely with gifs:

https://www.dropbox.com/sh/xptxt6skjgi0mjj/AAAfR3MrRxQpE5hZpOEm8UqAa

I can post the urls to these pages if that helps.

Background on which pages have been tested: A fresh D8 install is being done with all modules enabled. Then for every page SiteEffect finds in its build in crawler, the menu router item will be looked up. Only three pages per menu router item will be analyzed. This way only three menu edit forms will be tested and not hundreds. The problem so far is though that no test nodes, comments or terms exist and therefore a lot isn't being covered. Working on it...

The test setup is not fully automated yet, so it always takes me a little while, but I'm getting there. If I can help with running more tests, maybe on the latest patch then let me know.

YesCT’s picture

Issue summary: View changes
Issue tags: +Needs issue summary update, +Needs reroll

lets get a beta evaluation for this normal task. added directions to the issue summary.

vks7056’s picture

FileSize
0 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,929 pass(es). View

I added patch, Please check.

vks7056’s picture

FileSize
17.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,918 pass(es), 12 fail(s), and 0 exception(s). View

I added patch, Please check.

Status: Needs review » Needs work

The last submitted patch, 94: remove-title-wrapper-div-theme_item_list-1842140-63.patch, failed testing.

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia

#94 is incomplete, rerolling...

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
Status: Needs work » Needs review
FileSize
20.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,985 pass(es), 2 fail(s), and 0 exception(s). View

rerolled #83.

Status: Needs review » Needs work

The last submitted patch, 97: remove-title-wrapper-div-theme_item_list-1842140-97.patch, failed testing.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
20.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

$expected wasnt reinitialized... oops!

Manuel Garcia’s picture

FileSize
20.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,980 pass(es). View

Argh left me debug calls in the last patch sorry.

The last submitted patch, 99: remove-title-wrapper-div-theme_item_list-1842140-99.patch, failed testing.

manningpete’s picture

Issue tags: -Needs reroll

Patch applies

YesCT’s picture

Issue summary: View changes

@Manuel Garcia Do not worry about the multiple patches. :)

Interdiffs would be nice when making changes after a patch that applies when getting tests to pass though.
For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff
For example you could have made a interdiff.1842140.97.99.txt and interdiff.1842140.99.100.txt
it helps other people learn from your fixes, and helps reviewers.

Please let us be sure to get the screenshots and issue summary done while this applies and passes tests.

Manuel Garcia’s picture

@YesCT true... I keep forgetting we can just use interdiff for this, will tattoo it on me forhead.

mdrummond’s picture

Glad to see the rerolling. I think we still need to address the concerns I raised in #90 with using elements in the CSS selectors rather than classes, as that goes against our CSS architecture for D8.

BarisW’s picture

@mdrummong; the patch replaces the logic with ID's as-is. This issue is targeted at removing the wrapper div, which it does. Can we add a follow-up to improve the css selectors?

BarisW’s picture

FileSize
19.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,444 pass(es), 7 fail(s), and 0 exception(s). View

Core moves fast. Here is a re-roll.

Status: Needs review » Needs work

The last submitted patch, 107: remove-title-wrapper-div-theme_item_list-1842140-107.patch, failed testing.

BarisW’s picture

Status: Needs work » Needs review
FileSize
20.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,451 pass(es). View
1.35 KB

Try again, testbot.

Cottser’s picture

Status: Needs review » Needs work

Regarding #106: In my opinion, no. At this stage in the D8 release cycle, we really can't make D8 less shippable. Even ignoring that, we should follow our own standards.

Thanks for keeping on this @BarisW.

ti2m’s picture

I tested the patch in #109 again with siteeffect, looks lot more stable now. Some issues I found:

  • /admin/help: The spacing changes a lot, not sure if that is ok
  • /admin/reports/updates: Two new bullet points for 'Release notes' and 'Download' that are totally off
  • /admin/structure/views/view/frontpage: "Add content" in the preview now has a bullet point that even sticks outside the preview
  • The whole admin menu has different spacing
  • Primary tabs have a different spacing
rteijeiro’s picture

Hey @ti2m, it would be nice if you can provide some screenshots from your awesome SiteEffect tool to help to find the issues.

ti2m’s picture

Here are some screenshots, gifs don't work that well either. I think best might be to generate screenshots next to each other and highlight changed areas. Maybe this works for now if you download them and switch between them.

https://www.dropbox.com/sh/000977owkn9uuae/AAD3Pf8M4HqA-S96GzGenJ2ha?dl=0

akalata’s picture

Issue tags: +Needs reroll
Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia

Rerolling

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned

Stuf taht was removed in core/modules/system/css/system.theme.css was now removed on core/themes/classy/css/components/item-list.css, but there are some styles left in there so someone should take a look.

As a matter of fact, please review everything to do with item-list.css both in bartik and classy.

I'm sorry but I messed up while rerolling so I'm not uploading the reroll. Will try getting to this at another time.

akalata’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
20.93 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,303 pass(es), 13 fail(s), and 0 exception(s). View

Posting a reroll.

Status: Needs review » Needs work

The last submitted patch, 117: remove-title-wrapper-div-theme_item_list-1842140-117.patch, failed testing.

The last submitted patch, 117: remove-title-wrapper-div-theme_item_list-1842140-117.patch, failed testing.

jibran’s picture

+++ b/core/modules/views_ui/css/views_ui.admin.theme.css
@@ -638,10 +638,15 @@ td.group-title {
+<<<<<<< HEAD
...
+=======
...
+>>>>>>> #109

@@ -752,7 +757,57 @@ td.group-title {
+<<<<<<< HEAD
...
+=======
...
+>>>>>>> #109

+++ b/core/themes/seven/css/components/menus-and-lists.css
@@ -17,12 +17,12 @@
   list-style-type: circle;

Merge art effects

akalata’s picture

Issue tags: +Needs reroll

thanks @jibran! I think you meant "artifacts" here, instead of "art effects" :)

I see what Manuel means in #116, reconciling the changes to Bartik's item-list.css and system.theme.css is making my head hurt!

Manuel Garcia’s picture

Yeah @akalata... i messed it up there as well... I'm pinging the fine folks on #drupal-twig in IRC to see if any of them can give us some information to reroll this properly. Will post here any findings.

lauriii’s picture

Status: Needs work » Needs review
FileSize
21.94 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,439 pass(es), 13 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 123: remove_title_and-1842140-123.patch, failed testing.

The last submitted patch, 123: remove_title_and-1842140-123.patch, failed testing.

rootwork’s picture

  1. +++ b/core/themes/bartik/css/components/item-list.css
    @@ -3,24 +3,25 @@
    -.item-list ul {
    +ul {
       list-style: none;
       margin: 0 0 0.25em 0;
       padding: 0; /* LTR */
     }
    

    This is going to apply to EVERY ul element. Is that really what we want?

    Similarly the styles in the patch for li are going to affect all of them.

  2. +++ b/core/themes/seven/css/components/menus-and-lists.css
    @@ -1,15 +1,15 @@
    +ul {
       list-style-type: disc;
       list-style-image: none;
       margin: 0.25em 0 0.25em 1.5em; /* LTR */
     }
    

    This will also apply to every ul in Seven.

  3. I don't really understand why we can't put reasonable semantic class names on these elements and refer to them as such. Targeting elements with CSS seems like a bad idea. I support the idea of removing the wrapper div, but why can't we move the wrapper div's class -- item-list -- to the ul itself? That's what the issue summary says is supposed to happen.

  • alexpott committed 7939567 on 8.1.x
    Issue #1842140 by mdrummond, tonystar, gavin.hughes, killerpoke, lauriii...
  • alexpott committed ad5024c on 8.1.x
    Revert "Issue #1842140 by mdrummond, tonystar, gavin.hughes, killerpoke...
rteijeiro’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
20.88 KB

Re-rolled. Probably #126 should be addressed

Status: Needs review » Needs work

The last submitted patch, 128: remove_title_and-1842140-128.patch, failed testing.

The last submitted patch, 128: remove_title_and-1842140-128.patch, failed testing.

Manjit.Singh’s picture

Version: 8.0.x-dev » 8.2.x-dev
Issue tags: +Needs reroll
Manjit.Singh’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
20.9 KB

Status: Needs review » Needs work

The last submitted patch, 132: remove_title_and-1842140-132.patch, failed testing.

  • alexpott committed 7939567 on 8.3.x
    Issue #1842140 by mdrummond, tonystar, gavin.hughes, killerpoke, lauriii...
  • alexpott committed ad5024c on 8.3.x
    Revert "Issue #1842140 by mdrummond, tonystar, gavin.hughes, killerpoke...

  • alexpott committed 7939567 on 8.3.x
    Issue #1842140 by mdrummond, tonystar, gavin.hughes, killerpoke, lauriii...
  • alexpott committed ad5024c on 8.3.x
    Revert "Issue #1842140 by mdrummond, tonystar, gavin.hughes, killerpoke...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • alexpott committed 7939567 on 8.4.x
    Issue #1842140 by mdrummond, tonystar, gavin.hughes, killerpoke, lauriii...
  • alexpott committed ad5024c on 8.4.x
    Revert "Issue #1842140 by mdrummond, tonystar, gavin.hughes, killerpoke...

  • alexpott committed 7939567 on 8.4.x
    Issue #1842140 by mdrummond, tonystar, gavin.hughes, killerpoke, lauriii...
  • alexpott committed ad5024c on 8.4.x
    Revert "Issue #1842140 by mdrummond, tonystar, gavin.hughes, killerpoke...

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.