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.
Comment | File | Size | Author |
---|---|---|---|
#59 | interdiff.txt | 568 bytes | mr.baileys |
#59 | twig-views-view-grid-1843750-59.patch | 4.85 KB | mr.baileys |
#53 | interdiff.txt | 1.39 KB | shanethehat |
#53 | twig-views-view-grid-1843750-53.patch | 4.88 KB | shanethehat |
#47 | interdiff.txt | 2.41 KB | joelpittet |
Comments
Comment #1
joelpittetNot sure about these:
{{ column_classes[row_number][column_number] }}
{{ row_classes[row_number] }}
maybe they should be added in the preprocess using attributes?
Comment #2
mbrett5062 CreditAttribution: mbrett5062 commentedTagging.
Comment #3
DaneMacaulay CreditAttribution: DaneMacaulay commentedWe should hold off until progress on: #1903746: Replace the views grid table template with one using divs
is done
Comment #4
FluxSauce CreditAttribution: FluxSauce commentedRe-rolled with new location (views templates, not stark) and some corrections. I see the note on #1903746: Replace the views grid table template with one using divs; with that said, here's the work that was done (1:1 conversion) and steveoliver or someone else can decide how to proceed for the time being.
Comment #5
FluxSauce CreditAttribution: FluxSauce commentedComment #6
gollyg CreditAttribution: gollyg commented#5 seems to introduce an extra '.' into the code.
Removed the extra dot and re-rolled.
Comment #7
jpamental CreditAttribution: jpamental commentedNew version of patch that fixes inclusion of row and column classes. Includes a patch to views.theme.inc to put the row classes into $vars['row_classes']
(supercedes #6 - that extra '.' didn't solve adding the correct row/cell classes)
Comment #8
dawehnerMaybe it would make sense to postpone this issue until #1903746: Replace the views grid table template with one using divs got some progress?
Comment #9
jpamental CreditAttribution: jpamental commentedI think I disagree. Our goal is 1:1 conversion, and to be honest I think the effort to convert to div (or more appropriately article) would require an easy change here but a bunch of changes in the CSS, making it a lot more complicated to resolve. This will at least get us a sorted out conversion of the existing method.
Once the conversion is done I'd be more than happy to work on converting to more modern HTML & CSS - but that will also likely want work on the preprocess functions too. A much bigger mess to clean up.
Comment #10
dawehnerThis menations everything twice (attributes, attributes.class), but not the other variables like $view which should be available as well.
Comment #11
jpamental CreditAttribution: jpamental commented@dawehner - I left it referencing both the main attributes and specifically the classes as they're used in different ways (and it was in there before, so seemed to make sense)
re: $view - it's not mentioned in the original PHP template, only $rows (or just rows in the new syntax).
Working on an updated patch for the core views module instead of Stark now.
Comment #12
jpamental CreditAttribution: jpamental commentedHere's a patch against a clean D8 core. Patches views.theme.inc, creates views-view-grid.html.twig and removes views-view-grid.tpl.php
Comment #13
jpamental CreditAttribution: jpamental commentedMoving to Drupal core project
Comment #15
jpamental CreditAttribution: jpamental commentedResubmitting. Had to update a preprocess function to enable responsive classes on grid-style output.
Comment #16
jpamental CreditAttribution: jpamental commentedSetting back to 'needs review'
Comment #17
joelpittetAdding twig tag
Comment #18
jastraat CreditAttribution: jastraat commentedComment #19
jastraat CreditAttribution: jastraat commentedRe-rolled patch and updated it to be in line with the twig coding standards.
Comment #20
star-szr@jastraat - Thanks! An interdiff to show your changes would be great, I know there aren't any on this issue just yet :)
Comment #22
jastraat CreditAttribution: jastraat commentedIs there a way to do an interdiff without manually re-making all the changes from the 2nd patch?
Comment #23
star-szrYes, good question :) I would use two local branches from 8.x, one for #15 and one for #19, and then diff the two branches to create the interdiff.
Something like this should do it:
Create first branch:
Create second branch:
Then while still on the second branch:
Or while on any branch:
Comment #24
jastraat CreditAttribution: jastraat commentedThat was a great answer! Wow. Ok - interdiff attached. (Not sure why the last patch didn't pass tests though.)
Comment #25
jastraat CreditAttribution: jastraat commented#19: drupal_twig_views_grid_template-1843750-19.patch queued for re-testing.
Comment #26
dawehner"to display the rows in a grid?"
I guess we always should have full sentences?
Can you explain that bit?
Please let's fix the spaces.
Comment #27
joelpittet@dawehner Most of us don't know the variable's purposes, only that they exist and should be documented. But these are not the focus of the conversion to twig only a happy documenting by product.
The variables unfortunately haven't been documented before conversion. It would be nice to have a hand from the VDC team to fill in the gaps. Maybe I can convert these to @todo for them later? The goal we have is to finish twig conversions to "working state" by mid April.
Comment #28
dawehnerI'm totally fine with that! Better to have an @todo, then no documentation.
It would be great if you could answer on
Comment #29
jastraat CreditAttribution: jastraat commented@dawehner - I almost removed that comment line since I didn't really understand the purpose either, but it was in the original patch that I was re-rolling. If no one knows why it was added, I'm happy to take it out.
Comment #30
jastraat CreditAttribution: jastraat commentedComment #31
star-szrTagging.
Comment #32
jastraat CreditAttribution: jastraat commentedI'm afraid this needs some more work. I'm not sure how the $responsive variable is intended to be determined, but currently it's undefined. I looked at how the view table template was handling this, but apparently individual fields in tables have a setting for responsive; it doesn't look like this exists for grid.
Could someone explain how $responsive should be determined in a grid context?
Comment #33
dawehnerThe responsive bits should be removed, as they should be only applied to real tables. The grid here has to be replaced by divs at some point.
Comment #34
jastraat CreditAttribution: jastraat commentedRemoved the responsive bits from the preprocess function. Also updated the documentation for the $variables array since the original patch documentation was based on an example that used a render element. (see http://drupal.org/node/1912606#comment-7275644)
Comment #35
thedavidmeister CreditAttribution: thedavidmeister commented+ * Default simple view template to display the rows in a grid.
Capital "V" for View.
Please remove any reference to PHP data types from Twig template files.
+ $variables['attributes_array'] = array('summary' => $handler->options['summary']);
What is this? attributes_array isn't used or documented in the Twig template. Do we need it?
Also, we need an
@ingroup themeable
for the template as well as@see template_preprocess()
and@see template_preprocess_views_view_grid()
Comment #36
star-szrAnd I'll add: we are removing
@ingroup views_templates
, see #1843738-13: [meta] Convert views module to Twig.Thanks for keeping on this @jastraat :)
Comment #37
jastraat CreditAttribution: jastraat commented@thedavidmeister - I don't know what the purpose of $variables['attributes_array'] is, but it was in the original preprocess function that we're converting. Anyone else want to chime in if they know?
Comment #38
thedavidmeister CreditAttribution: thedavidmeister commented#37 - would you be able to provide an example dump of what's in that variable by the end of the preprocess, with a print_r() or something?
my gut tells me we probably don't need it anymore as all attributes should be in the attributes variable but we should have a look what's inside first.
Comment #39
joelpittet@jastraat and @thedavidmeister I am quite sure that is a bug, I fixed it in #1843766: Convert views/templates/views-view-table.tpl.php to twig with
$vars['attributes']['summary'] = $handler->options['summary'];
I made a table view and tested the output and it is definitely broken in core.
It's an option in the views UI for the table summary attribute. Though grid will later become not a table, so this will be removed.
Comment #40
joelpittetCleanup, and revert $vars=>$variables because we have an issue for that #1963942: Change all instances of $vars to $variables and keeps it easier to see the other changes in the conversion patch.
Comment #41
joelpittethmm wrong patch... try that again.
Comment #42
dawehnerGreat, thank you!
Comment #43
thedavidmeister CreditAttribution: thedavidmeister commentedComment #44
Eric_A CreditAttribution: Eric_A commentedThe $attributes_array lines in the Twig conversion issues conflict with #1484704: Remove instances of attributes_array, so we'll need to re roll here or there anyway. Twig conversion issues will become easier to RTBC if the non-Twig issues go in first. (See also #93 in #1183042: Regression: Add WAI-ARIA roles to Core blocks)
@dawehner: #1484704: Remove instances of attributes_array is basically views only. Care to RTBC that one as well?
Comment #45
star-szrTagging for profiling.
Comment #46
star-szrI think this one still needs manual testing.
Here are some doc tweaks/updates:
Can we fill in something here - maybe one of the other views patches has this?
s/themable/themeable
@param array $vars for now for the preprocess function.
Again, I'm sure we can pull this from another views conversion patch.
Comment #47
joelpittetDoc updates from #46 plus a couple more.
Comment #49
shanethehat CreditAttribution: shanethehat commented#47: 1843750-47-twig-views-view-grid.patch queued for re-testing.
Comment #50
thedavidmeister CreditAttribution: thedavidmeister commentedi'll do some manual testing.
Comment #51
thedavidmeister CreditAttribution: thedavidmeister commentedHEAD:
#47:
Diff:
New HTML is functionally identical and has better whitespace. Twig conversion also fixes some a bunch of errors that were appearing for me in HEAD:
Notice: Undefined index: header in template_preprocess_views_view_grid() (line 769 of core/modules/views/views.theme.inc).
Notice: Undefined variable: row_classes in include() (line 19 of core/modules/views/templates/views-view-grid.tpl.php).
Notice: Undefined variable: row_classes in include() (line 19 of core/modules/views/templates/views-view-grid.tpl.php).
Comment #52
thedavidmeister CreditAttribution: thedavidmeister commented+ * - rows: A list of rows. Each row contains an list of columns.
"a list of columns."
+ // This should be string so that's okay.
"This should be a string so this is ok."
Comment #53
shanethehat CreditAttribution: shanethehat commentedDone and done
Comment #54
thedavidmeister CreditAttribution: thedavidmeister commentedComment #55
geoffreyr CreditAttribution: geoffreyr commentedProfiling.
Comment #56
geoffreyr CreditAttribution: geoffreyr commentedhttp://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519be9a13e594&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519be9a13e594&...
Comment #57
alexpottActually $result is not used so can be removed.
Comment #58
mr.baileysI'll take care of re-rolling.
Comment #59
mr.baileysFixed #57
Comment #60
mr.baileysComment #61
star-szrLooks good, thanks @mr.baileys! Great catch @alexpott :)
Comment #62
alexpott+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Comment #63
alexpottCommitted d39eace and pushed to 8.x. Thanks!
Comment #64.0
(not verified) CreditAttribution: commentedUpdated issue summary.