Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Patch to commit: #3
Problem/Motivation
- #1276908: Administrative tables are too wide for smaller screens introduced the concept of "responsive" tables but did not modify or add any tests to verify that the classes were added.
Proposed resolution
- Add some tests to ensure #806982-86: Tables should take an optional footer variable and produce <tfoot> doesn't happen again.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#3 | interdiff-2282683-1-4.txt | 3.46 KB | RainbowArray |
#3 | 2282683-4-responsive-table-test.patch | 3.08 KB | RainbowArray |
Comments
Comment #1
RainbowArrayHere's a few tests to check if responsive table classes are being applied correctly.
Comment #3
RainbowArrayHere's a revised patch with the interdiff.
Comment #4
RainbowArrayComment #5
markhalliwellWe should test that the priority classes aren't applied when
#responsive
isn't set or notTRUE
.Comment #6
RainbowArrayAdded the additional test.
Comment #8
RainbowArrayTried a different approach to check how output is handled when responsive tables are not enabled.
Comment #10
gnugetHi mdrummond.
You have done an awesome job with these tests, thank you.
Right now, the:
#responsive => TRUE
only add the class 'responsive-enabled' in the table tag and thecore/drupal.tableresponsive
library.Which i think it is totally ok, so in the tests the
#responsive => FALSE
only should check if theresponsive-enabled
class and the library are added, it is not necessary to make sure the priority-low and priority-medium classes are there because these are a common CSS classes, and the user must be able to add those classes even if not want a responsible table.The problem here is in the themes we have something like this:
it doesn't matter if we have
#responsive => FALSE
the priority-low and priority-medium classes will still be hiding in narrow devices.I think we should change that for something like this:
But that is out of the scope of the issue.
For now, In my opinion is only necessary change how
#responsive => FALSE
is tested.Comment #11
joelpittet@gnuget I agree it should be a bit more specific so we aren't name collision. Maybe @LewisNyman can chime in on #10?
Comment #12
LewisNymanI think it just testing the
.responsive-enabled
and the library is enough for this issue.I think the problem with the CSS classes is that it is not clear what the
.priority-x
classes do, so there is potential for unintentional overlap and false positives.Our CSS standards have a way of dealing with this :) I would suggest:
.responsive-priority-low
.responsive-priority-medium
Removing the elements from the selectors would also be nice.
Comment #13
RainbowArrayIf we have a property that turns responsiveness on tables on and off, then it doesn't make sense to enable responsiveness unless that responsive-enabled class is present. The suggestion in #10, minus the elements, would maybe be good:
I'm fine if we change that to:
That's outside the scope of this issue, but could be done in a follow-up side issue.
I'm still not clear why the latest tests I wrote failed. Any insights?
Comment #14
gnugetYour tests fails because the classes are added even if the
'#responsive' => FALSE,
is set... this is because those are normal classes and aren't stripped when the #responsive is false.So, here you only need to tests if the table has the responsive-enabled class, and that's it.
Comment #15
markhalliwellMy comment in #5 was based loosely on very tired IRC conversation between @mdrummond and myself. It was my understanding that this is how it should work, but ultimately if it failed a follow-up should be created to address it. As far as the class name changes, that is indeed out of scope and should be a separate follow-up issue.
The tests in #3 is sufficient for #806982: Tables should take an optional footer variable and produce <tfoot>.
Comment #16
LewisNymanTests are still failing :( See #14
Comment #17
markhalliwellIt already has that test, review #3.
Comment #18
webchickAwesome work! I'll sleep better at night with this patch. :)
Committed and pushed to 8.x. Thanks!