Issue #2152231 by steveoliver, joelpittet, hussainweb, shanethehat, jenlampton, kpa, AnythonyR, EVIIILJ, kgoel, Cottser, dsdeiz, hanpersand: Convert theme_vertical_tabs() to Twig

Task

Convert theme_vertical_tabs() to a Twig template.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing
  • Profiling

Steps to test

@todo

Files: 

Comments

Cottser’s picture

Issue summary: View changes

Adding a commit message to the issue summary so the folks who already worked on #1898480: [meta] form.inc - Convert theme_ functions to Twig and in the Twig sandbox get credit.

rteijeiro’s picture

Status: Active » Needs review
FileSize
2 KB
PASSED: [[SimpleTest]]: [MySQL] 59,427 pass(es). View

Let see...

sheilaj’s picture

Tested at /block/add. The markup before and after applying the patch (in #2) is identical. See attached screenshots of markup before and after, including twig debug code.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs profiling

Scenario: same as markup tested.

/block/add form on homepage with permissions set for anonymous.

=== 8.x..8.x compared (52e45e71d26a9..52e45eb0a8ef3):

ct  : 42,768|42,768|0|0.0%
wt  : 368,816|368,867|51|0.0%
cpu : 364,642|364,805|163|0.0%
mu  : 30,773,104|30,773,104|0|0.0%
pmu : 30,824,840|30,824,840|0|0.0%

Profiler output

=== 8.x..2152231-twig-theme-vertical-tabs compared (52e45e71d26a9..52e45eda7d593):

ct  : 42,768|42,846|78|0.2%
wt  : 368,816|369,715|899|0.2%
cpu : 364,642|365,600|958|0.3%
mu  : 30,773,104|30,804,600|31,496|0.1%
pmu : 30,824,840|30,855,760|30,920|0.1%

Profiler output

Profiling looks good, RTBC:)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks! Another one bites the dust. :D

joelpittet’s picture

Woo, thanks @webchick and @sheilaj for joining the sprint!

Cottser’s picture

Title: Convert theme_vertical_tabs() to Twig » Convert theme_vertical_tabs() to Twig [small followup]
Status: Fixed » Needs work
Issue tags: +Novice

Normally I'd open a new issue for follow-up but this is so minor that it would not be worth it.

+++ b/core/modules/system/templates/vertical-tabs.html.twig
@@ -0,0 +1,15 @@
+<div data-vertical-tabs-panes {{ attributes }}>{{ children }}</div>

There should never be a space before we print attributes, that is automatically inserted so that we can do things like <div{{ attributes }}> and things will work out whether there are attributes or not.

Having this extra space results in markup like this:

<div data-vertical-tabs-panes  class="entity-meta">
<div data-vertical-tabs-panes >

Making this small change and creating a patch would be a great task for a new contributor or someone who worked on this over the weekend.

InternetDevels’s picture

Status: Needs work » Needs review
FileSize
460 bytes
PASSED: [[SimpleTest]]: [MySQL] 63,327 pass(es). View
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice

Better, thanks.

Cottser’s picture

Issue tags: +SprintWeekend2014

Adding sprint weekend tag. Thanks @InternetDevels for the fast turnaround!

joelpittet’s picture

joelpittet’s picture

Issue tags: +SprintWeekend2014

Sorry, about the sprint weekend tag removal, thought we weren't supposed to add those...

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.