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.
Meta: #1843738: [meta] Convert views module to Twig
To test this code
- Create or edit a view
- Change the Format of the view to table.
- Configure the Settings of that format.
- - this settings form should be using the new template: views-ui-style-plugin-table.html.twig
Comment | File | Size | Author |
---|---|---|---|
#27 | interdiff.txt | 3.37 KB | star-szr |
#27 | 1918648-27.patch | 4.6 KB | star-szr |
#24 | twig-convert_views_ui-1918648-24.patch | 5.13 KB | jenlampton |
#24 | views-ui-before.png | 129.25 KB | jenlampton |
#24 | views-ui-after.png | 129.15 KB | jenlampton |
Comments
Comment #1
steveoliver CreditAttribution: steveoliver commentedComment #2
damiankloip CreditAttribution: damiankloip commentedHowcome we aren't using localized strings here? I think you need to do this for table headers
Comment #3
star-szrTagging.
Comment #4
joelpittetRe-roll, added t() call's back in and changed to type=>table and removed render calls.
Comment #5
myke CreditAttribution: myke commentedPatch in #4 seems to work, I applied it and tested it out... I did not compare the HTML output of the patched version vs. the non-patched version.
Comment #6
thedavidmeister CreditAttribution: thedavidmeister commentedCode style for #4 looks good to me.
+ $variables['description'] = $form['description_markup'];
Should we unset $form['description_markup'] after this line?
This issue needs some manual testing steps written up in the issue summary and for someone to review the markup.
Comment #7
Fabianx CreditAttribution: Fabianx commented#4: twig-views-ui-style-plugin-table-1918648-4.patch queued for re-testing.
Comment #9
star-szrTagging for reroll, the Views UI module got moved to core/modules in #1820414: CHANGE NOTICE: Move views_ui.module directly into /core/modules/.
Comment #10
chrisjlee CreditAttribution: chrisjlee commentedReroll attempt; luckily no merge conflicts.
Comment #11
chrisjlee CreditAttribution: chrisjlee commentedWhoops forgot to move the html.twig template to "/core/modules/views_ui/templates" from "core/modules/views/views_ui/templates/"
Comment #12
intergalactic overlords CreditAttribution: intergalactic overlords commentedSmall mistake in the twig version. The discription above the table is repeated at the bottom of the modal view. See screenshot.
Comment #13
mr.baileysI'll take care of #12 as part of the DC Portland Friday sprint.
Comment #14
mr.baileysPatch attached.
Comment #15
alexdmccabeAssigning it to myself for review.
Comment #16
alexdmccabeLooks good to me!
Comment #17
damiankloip CreditAttribution: damiankloip commentedThis also needs vdc review. I'll take a look this morning.
Comment #18
damiankloip CreditAttribution: damiankloip commentedThis should just use a row span here? Would be a much more elegant solution.
Comment #20
mr.baileysAgreed, patch attached.
Comment #21
mr.baileysInterdiff to go with previous patch.
Comment #22
joelpittetNeeds profiling and manual testing.
Thanks @mr.baileys, @alexdmccabe and @damiankloip!
Comment #23
thedavidmeister CreditAttribution: thedavidmeister commentedManual testing is a novice task.
Comment #24
jenlamptonneeded a reroll.
Also, did some manual testing and it looks like our output is solid.
Before:
After:
Comment #24.0
jenlamptontest
Comment #25
star-szrI will profile this one over the weekend.
Comment #26
star-szrThanks @jenlampton :)
I did another quick markup comparison with visual diff and DaisyDiff, the only markup that changed was the colspans and that was OK'd by @damiankloip in #18.
I found a few very minor documentation and code style issues but I also thought it might be worth rethinking all the unset()'s and remove an unnecessary variable set/unset in preprocess by accessing the variable directly in the Twig template. Markup still matches up and less unset()'s reduces the function calls significantly, increasing performance.
I profiled the following page:
admin/structure/views/nojs/display/content/page_1/style_options
Baseline vs. baseline
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5208407650493&...
Patch from #24
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5208407650493&...
This patch
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5208407650493&...
Comment #27
star-szrPatch :)
Comment #28
joelpittetLooks like this one is solid, nice work!
@Cottser could you add your scenario as well please?
Comment #29
webchickGood stuff!
Committed and pushed to 8.x. Thanks!
Would be great if there was some way to make this not a special template, and instead just using table.twig.html in a follow-up.
Comment #30
star-szr@joelpittet - I didn't call much attention to it but there's not much to the scenario, the URL is it. Default install and profiled the following page (exposed views admin to anonymous users):
admin/structure/views/nojs/display/content/page_1/style_options
Comment #31
joelpittetThanks, thought there would be more but that's easier:)
Comment #32.0
(not verified) CreditAttribution: commentedli