Issue #2152207 by steveoliver, joelpittet, gnuget, idflood, hussainweb, shanethehat, jenlampton, kpa, AnythonyR, EVIIILJ, kgoel, Cottser, dsdeiz, hanpersand: Convert theme_details() to Twig

Task

Convert theme_details() to a Twig template.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing
  • Profiling

Steps to test

Visit /admin/appearance/settings
Review the form output of the <detail> tags

Files: 
CommentFileSizeAuthor
#18 interdiff.txt4.22 KBjoelpittet
#18 2152207-twig-theme-details-18.patch11.13 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 63,190 pass(es). View
#14 2152207-6-theme_details.patch11.16 KBgnuget
PASSED: [[SimpleTest]]: [MySQL] 59,845 pass(es). View
#4 interdiff.txt5.34 KBjoelpittet
#4 2152207-4-theme_details.patch8.02 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 59,617 pass(es). View
#2 2152207-2-twig-theme_details.patch5 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 59,362 pass(es), 4 fail(s), and 0 exception(s). View

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.

joelpittet’s picture

Status: Active » Needs review
FileSize
5 KB
FAILED: [[SimpleTest]]: [MySQL] 59,362 pass(es), 4 fail(s), and 0 exception(s). View

Split from form.inc twig conversion.

Status: Needs review » Needs work

The last submitted patch, 2: 2152207-2-twig-theme_details.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.02 KB
PASSED: [[SimpleTest]]: [MySQL] 59,617 pass(es). View
5.34 KB

Fixed the whitespace errors and did some clean-up.

John Pitcairn’s picture

Shouldn't there be a div.description in here?

+    <div class="details-wrapper">
+      {%- if description -%}
+        <div class="details-description">{{ description }}</div>
+      {%- endif -%}
+      {%- if children -%}
+        {{ children }}
+      {%- endif -%}
+      {%- if value -%}
+        {{ value }}
+      {%- endif -%}
+    </div>
+  </details>

div.details-description does not receive the same text formatting as div.description, which makes form help text look inconsistent, and requires extra work for themes to override.

If there is a div.details-wrapper I'd expect to find a div.details inside that, or else the wrapper should be div.details-description-wrapper.

I think ideally this should be changed to:

<div class="details description">

There are only two lines in core css targeting .details-description (in user.module.css and bartik style.css) - these would need to change to .details.description. Any objections?

joelpittet’s picture

@John Pitcairn those are great suggestions though not particularly for this issue. Have a look here and help @mortendk #1980004: [meta] Creating Dream Markup if there is not for theme_details() an issue, you're welcome to create it and add it to the meta with exactly what you wrote in the comment above.

This issue is related to getting the conversion of a theme_* function to a twig file. We won't introduce markup or style changes as we are comparing that we haven't made a mistake in the conversion process.

If you want to help with this issue still, we need someone to compare the markup before and after the patch is applied to make sure we didn't break anything existing. That would help get this theme_ function die and get the template as twig in core so it would be easier for you to manipulate the markup to your liking.

John Pitcairn’s picture

Thought somebody might say that ;-)

Will do. Sorry I can't test this immediately, we are building a site on alpha 7.

Issue is here: #2177813: details.html.twig should include .description class

joelpittet’s picture

Issue summary: View changes
joelpittet’s picture

Status: Needs review » Needs work
FileSize
44.51 KB
+++ b/core/modules/system/templates/details.html.twig
@@ -0,0 +1,35 @@
+{% spaceless %}
...
+      {%- endif -%}
+      {%- if children -%}
+        {{ children }}
+      {%- endif -%}
+      {%- if value -%}
+        {{ value }}
+      {%- endif -%}

Due to either spaceless of the minus whitespace removers there is missing space between input's and their labels. My guess is spaceless. I'd rather not use spaceless, but that means fixing the whitespace in the tests.

settings

gnuget’s picture

Assigned: Unassigned » gnuget
FileSize
7.95 KB
FAILED: [[SimpleTest]]: [MySQL] 59,737 pass(es), 4 fail(s), and 0 exception(s). View
1.34 KB

I'm going to upload a version without the spaceless tag for see exactly which tests fails

After that i'm going to work on fix them.

joelpittet’s picture

Status: Needs work » Needs review

Starting up the testbot (needs review). To see if there are any tests we need to fix. Hopefully we can do xpath equivalent tests to help ignore whitespace differences. Thanks @gnuget

The last submitted patch, 10: 2152207-5-theme_details.patch, failed testing.

joelpittet’s picture

@gnuget the tests are in-line with the ones you found. It says "ForumUninstallTest" too but I doubt that fail is real(bot anomaly)...

Buena suerte!

gnuget’s picture

Assigned: gnuget » Unassigned
FileSize
11.16 KB
PASSED: [[SimpleTest]]: [MySQL] 59,845 pass(es). View
6.45 KB

Ok, here a patch which replace the failing tests with their xpath equivalents

MartiMcFlight’s picture

The rendered code before the patch is the same as the rendered code after the patch. Only the order of some tag attributes and ids or classes has changed, but this does not break the applied css. The page looks exactly the same in the browser.

I haven’t done profiling but i’m not sure if it is necessary for this issue since the theme details won’t be changed often, so performance is not critical.

MartiMcFlight’s picture

Issue tags: +SprintWeekend2014, +D8MA
idflood’s picture

Issue summary: View changes

I profiled the patch in #14. I wasn't sure about the results so I did this multiple times and the results were always similar.

=== 8.x..8.x compared (52e3dfa285188..52e3dffbc04ba):

ct  : 62,040|62,040|0|0.0%
wt  : 377,813|378,779|966|0.3%
cpu : 360,984|361,903|919|0.3%
mu  : 15,181,856|15,197,528|15,672|0.1%
pmu : 15,312,040|15,326,864|14,824|0.1%


=== 8.x..details-twig compared (52e3dfa285188..52e3e019b34d3):

ct  : 62,040|62,242|202|0.3%
wt  : 377,813|376,456|-1,357|-0.4%
cpu : 360,984|359,907|-1,077|-0.3%
mu  : 15,181,856|15,229,112|47,256|0.3%
pmu : 15,312,040|15,358,808|46,768|0.3%

I've also looked at the markup and it looks ok (same as #15).

Now the only thing missing is a proper patch review.

joelpittet’s picture

Issue tags: -Needs profiling
FileSize
11.13 KB
PASSED: [[SimpleTest]]: [MySQL] 63,190 pass(es). View
4.22 KB

I reviewed the xpath and the patch and did some minor cleanup but I believe this captures all that is expected from the markup without the whitespace differences. Great work.

joelpittet’s picture

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

Oh I did a manual review on this already and reviewed the new xpath changes, so this is good to go. Testbot can disagree if it wants but I think this is RTBC.

joelpittet’s picture

Also thanks for profiling @idflood and the test changes @gnuget!

Cottser’s picture

Issue summary: View changes

Adding some folks to the proposed commit message :)

Cottser’s picture

Issue tags: +Twig conversion
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.