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.
Just creating a separate issue for a backport of #806982: Tables should take an optional footer variable and produce <tfoot>
Comment | File | Size | Author |
---|---|---|---|
#36 | interdiff.txt | 1.2 KB | Pol |
#36 | 1892654.patch | 6.77 KB | Pol |
Comments
Comment #1
willvincent CreditAttribution: willvincent commentedHere's a properly formed patch based on miqmago's malformed patch from http://drupal.org/node/806982#comment-6890454 (file paths are based on D8 directory structure in the patch).
Comment #2
willvincent CreditAttribution: willvincent commentedComment #3
marcingy CreditAttribution: marcingy commentedThe d8 version needs to be committed first. Once that happens d7 patch needs to be attatched to that issue.
Comment #4
joseph.olstadD8 patch was committed, Pol has an updated patch in that issue, copy that patch here as this is the new policy.
Comment #5
joseph.olstadComment #6
PolHere's the patch.
Comment #8
PolHere's the updated patch.
Comment #9
PolHere's a better patch with an improved coding style.
Comment #11
PolNew patch, including tests.
Comment #12
PolComment #13
joseph.olstadIt's already in D8, passes tests, has test, looks good.
Although I've never actually had to use this yet, others are.
Comment #14
joseph.olstadBumping to 7.70, this didn't make it into 7.60
Comment #15
apotek CreditAttribution: apotek commentedJust re-rolling the excellent patch from @Pol at comment #11 to match current state of drupal 7 (7.63) as the old patch no longer applies.
Endless rework of good work while waiting for the merge to master.
Comment #17
PolThe patch still apply on my side, without any troubles.
Comment #18
PolUpdating credits.
Comment #19
PolComment #20
PolComment #21
PolComment #22
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI'd like a little restructuring here:
That will simplify the code and make it easier to ready.
---
I delayed committing that as the changes looked massive, but it was overall just structure changes, which is okay this time, but next time let's please do cleanup separate from feature development as it makes things harder to review than necessary.
Comment #23
PolHi Fabian,
Thanks for replying.
We cannot do the change you requested.
The
$sections
variable is a mapping of keys from$variables
array into their respective HTML tags and they are processed using the same mechanism, this is why this is abstracted into this variable, to avoid code duplication.rows
=><tbody>
footer
=><tfoot>
If we implement your logic, we won't be able to map the
$rows
to<tbody>
tag and$footer
to<tfoot>
tag.Comment #24
PolAfter a discussion in IRC, I got the Fabian's point.
Here's an updated patch and the interdiff.
Comment #25
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedMy bad - we need to check for !empty() like before to populate 'rows' in sections.
Is there a reason for moving the no_striping out of the `!empty($cells)`?
Looks wrong - or rather needs a justification before that can go in.
The nice thing about this new approach is that the indentation almost remains the same, which makes this much simpler to review than before :D.
Comment #26
PolUpdated patch.
I moved back the
no_stripping
logic into the condition as it was before.Comment #27
PolUpdating credits - adding Fabian.
Comment #28
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedShould be !empty($rows) as that is the weird header / rows structure.
This change is problematic because it changes the current behaviour.
What would work for me is to keep the code as-is, but initialize the $no_stripiing once before the for loop if the tag is tfoot.
That would change the default just once.
This move is unnecessary.
Comment #29
PolThanks, updating the patch, interdiff provided.
Comment #30
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedLet's not remove the else - the additional line is fine though.
Comment #31
PolUpdating patch.
Comment #32
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI think it might be good to say that no_striping defaults to FALSE for 'rows' and TRUE for 'footer'.
Let's do this differently and put:
$default_no_striping = ($tag === 'tfoot');
Then use $default_no_striping both in the ternary operator as well as in the else block.
Comment #33
PolComment #34
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedjust nits remaining!
Looks very great already :).
s/stripping/striping/
This is no longer needed as it's unused.
I think we can change that to '// Format the table rows and footer'
Comment #35
PolForget this one, I forgot to fix a typo.
Comment #36
PolComment #38
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC - let's get that in
Comment #40
PolThanks all!
Comment #41
joseph.olstadNice work! Thanks!
Merci Pol
Dankeschön FabianX