Convert theme_views_ui_style_plugin_table to Twig

+++ b/core/modules/views/views_ui/views_ui.theme.incundefined
@@ -368,36 +368,36 @@ function theme_views_ui_rearrange_filter_form(&$vars) {
+    'Field',
+    'Column',
+    'Align',
+    'Separator',

Howcome we aren't using localized strings here? I think you need to do this for table headers

Tagging.

Re-roll, added t() call's back in and changed to type=>table and removed render calls.

Patch 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.

Code 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.

#4: twig-views-ui-style-plugin-table-1918648-4.patch queued for re-testing.

Tagging for reroll, the Views UI module got moved to core/modules in #1820414: CHANGE NOTICE: Move views_ui.module directly into /core/modules/.

Reroll attempt; luckily no merge conflicts.

Whoops forgot to move the html.twig template to "/core/modules/views_ui/templates" from "core/modules/views/views_ui/templates/"

Small mistake in the twig version. The discription above the table is repeated at the bottom of the modal view. See screenshot.

I'll take care of #12 as part of the DC Portland Friday sprint.

Patch attached.

Assigning it to myself for review.

Looks good to me!

This also needs vdc review. I'll take a look this morning.

+++ b/core/modules/views_ui/views_ui.theme.incundefined
@@ -400,22 +416,29 @@ function theme_views_ui_style_plugin_table($variables) {
+  $rows[] = array(t('None'), '', '', '', '', '', array('align' => 'center', 'data' => $form['default'][-1]), '', '');

This should just use a row span here? Would be a much more elegant solution.

Agreed, patch attached.

Interdiff to go with previous patch.

Needs profiling and manual testing.

Thanks @mr.baileys, @alexdmccabe and @damiankloip!

Manual testing is a novice task.

needed a reroll.
Also, did some manual testing and it looks like our output is solid.
Before:

After:

test

I will profile this one over the weekend.

Thanks @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

=== 8.x..8.x compared (5208407650493..520840a963a54):

ct  : 58,545|58,545|0|0.0%
wt  : 277,114|277,222|108|0.0%
cpu : 247,892|248,071|179|0.1%
mu  : 13,963,368|13,963,368|0|0.0%
pmu : 14,090,680|14,090,680|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5208407650493&...

Patch from #24

=== 8.x..views-ui-style-plugin-table-1918648-24 compared (5208407650493..52084105e6dcb):

ct  : 58,545|58,629|84|0.1%
wt  : 277,114|277,880|766|0.3%
cpu : 247,892|248,941|1,049|0.4%
mu  : 13,963,368|13,990,072|26,704|0.2%
pmu : 14,090,680|14,118,048|27,368|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5208407650493&...

This patch

=== 8.x..views-ui-style-plugin-table-1918648-26 compared (5208407650493..520840e157983):

ct  : 58,545|58,582|37|0.1%
wt  : 277,114|277,349|235|0.1%
cpu : 247,892|248,786|894|0.4%
mu  : 13,963,368|13,992,320|28,952|0.2%
pmu : 14,090,680|14,119,672|28,992|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5208407650493&...

Patch :)

Looks like this one is solid, nice work!

@Cottser could you add your scenario as well please?

Good 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.

@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

Thanks, thought there would be more but that's easier:)