This is a regression from: #1939008: Convert theme_table() to Twig.

Follow-ups from:
#806982: Tables should take an optional footer variable and produce <tfoot>
#2282683: Responsive tables do not have tests

Problem/Motivation

  • Classes are only added based on header indices, which match the row indices. This causes headers, which are associative, to not propagate the responsive classes to the rows, which are indexed.
  • Example of /admin/content/comment:

Proposed resolution

  • Use a separate counter (like it was before) and not the header index.

Remaining tasks

  • Create patch.

User interface changes

None.

API changes

None

CommentFileSizeAuthor
#2 responsive-headers-fixed.jpg423.76 KBmarkhalliwell
#1 regression_responsive-2296859-1-PASS.patch3.74 KBmarkhalliwell
#1 regression_responsive-2296859-1-FAIL.patch1.21 KBmarkhalliwell
responsive-headers.jpg399.18 KBmarkhalliwell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell commented
Issue summary: View changes
Status: Active » Needs review
FileSize
regression_responsive-2296859-1-FAIL.patch1.21 KB
regression_responsive-2296859-1-PASS.patch3.74 KB
markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell commented
FileSize
responsive-headers-fixed.jpg423.76 KB

Here's the screenshot of it fixed:

The last submitted patch, 1: regression_responsive-2296859-1-FAIL.patch, failed testing.

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell commented
Title: [regression] Responsive classes not always added to rows » [regression] Responsive classes not always added to table rows
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Hm. To me, this is a matter of architectural expectations.

My natural DX expectation would be

  1. If you use named array keys in the table header
  2. Then you also have to use named array keys in the table rows
  3. Otherwise, theme_table() blows up.

In other words, my DX expectation would rather be the "opposite" of this patch:

  1. Why is no exception thrown if a table column suddenly starts to use a numbered key in a table that uses named keys? (and vice-versa)
  2. Why is it possible that table header columns can use named keys but table rows can use indexed keys? (and vice-versa)

IMO, fixing the error/exception handling for the root cause (inconsistent column keys) would deliver a much better DX + reliability. Especially for alter hook operations that are adding columns/data to a certain table.

Thoughts?

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell commented

That would indeed be an improvement (one I would gladly follow/support). However this issue is about fixing an obvious currently broken HEAD caused by the conversion of table theme functions to twig. I do not wish to drag this particular issue out. Please open a follow-up.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: regression_responsive-2296859-1-PASS.patch, failed testing.

Mark Carver queued 1: regression_responsive-2296859-1-PASS.patch for re-testing.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs work » Reviewed & tested by the community
alexpott’s picture

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

Committed 5fe283c and pushed to 8.x. Thanks!

  • alexpott committed 5fe283c on 8.x 
    Issue #2296859 by Mark Carver: Fixed [regression] Responsive classes not...

Status: Fixed » Closed (fixed)

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