Issue #2151119 by joelpittet, Cottser, idflood, InternetDevels, c4rl, IshaDakota, pplantinga, gnuget, longwave, jeanfei, sbudker1: Convert theme_system_themes_page() to Twig

Task

Convert theme_system_themes_page() to a Twig template.

Remaining tasks

  • Patch
  • Patch needs another review
  • Manual testing
  • Profiling

Steps to test

Navigate to /admin/appearance.

Comments

star-szr’s picture

Issue summary: View changes

Adding a commit message to the issue summary so the folks who already worked on #1987410: [meta] system.module - Convert theme_ functions to Twig and #1898454: system.module - Convert PHPTemplate templates to Twig (comment #41 and above aka @c4rl) get credit.

joelpittet’s picture

Status: Active » Needs review
StatusFileSize
new9.63 KB

Split from the system module twig conversion meta.

Status: Needs review » Needs work

The last submitted patch, 2: 2151119-1-twig-theme_system_themes_page.patch, failed testing.

joelpittet’s picture

StatusFileSize
new9.64 KB
new705 bytes

typo fix.

star-szr’s picture

Status: Needs work » Needs review

Thanks @joelpittet!

ipo4ka704’s picture

The last submitted patch, 4: 2151119-4-twig-theme_system_themes_page.patch, failed testing.

joelpittet’s picture

Re-roll and leave the form alter were it was.

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

idflood’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new8.91 KB

Reroll of path in #8.

idflood’s picture

Issue summary: View changes
Issue tags: -needs profiling

Here is the profiling of the patch in #10.

=== 8.x..8.x compared (52e3e4975b356..52e3e4dce8a32):

ct  : 56,131|56,131|0|0.0%
wt  : 350,578|351,401|823|0.2%
cpu : 334,639|335,689|1,050|0.3%
mu  : 15,056,104|15,056,104|0|0.0%
pmu : 15,183,712|15,183,712|0|0.0%


=== 8.x..theme-system-twig compared (52e3e4975b356..52e3e5149d9ad):

ct  : 56,131|56,553|422|0.8%
wt  : 350,578|351,361|783|0.2%
cpu : 334,639|335,033|394|0.1%
mu  : 15,056,104|15,104,792|48,688|0.3%
pmu : 15,183,712|15,234,608|50,896|0.3%

I've also quickly checked visually and the markup of admin/appearance . Seems to be fine but I did not check for the case where a theme doesn't have a screenshot.

internetdevels’s picture

StatusFileSize
new6.77 KB
new535 bytes

Fixed comments standards in the previous patch.

Status: Needs review » Needs work

The last submitted patch, 12: twig-theme_system_themes_page-2151119-11.patch, failed testing.

star-szr’s picture

Thanks @InternetDevels, we weren't changing/adding that line so we could just as well leave it as is. We lost the .html.twig template from the patch though, so that needs to be added back :)

internetdevels’s picture

Status: Needs work » Needs review
StatusFileSize
new8.94 KB
new2.02 KB

Thanks @Cottser, for the answer.
Oops, we really missed it. Added it back ;)

star-szr’s picture

Issue tags: +Twig conversion
idflood’s picture

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

Did some manual testing and everything works as expected.

The patch looks good and the markup also doesn't have any significant difference.

star-szr’s picture

Status: Reviewed & tested by the community » Needs work

Thanks @idflood! I'm reviewing this and it's not quite ready yet, patch coming a bit later.

star-szr’s picture

Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new9.36 KB
new7.37 KB

Summary of changes:

  • Moves logic for theme compatibility and theme notes into the template. This allows for a better match with the incompatible theme markup (we were missing div.incompatible which Seven for example styles differently).
  • Cleans up docblock in template (minor nitpicks and more clearly shows the nested structure), other minor documentation tweaks.
  • Removes unused empty screenshot markup from preprocess since this is in the template now.

I compared markup again and it looks good, including the no screenshot case.

joelpittet’s picture

+++ b/core/modules/system/templates/system-themes-page.html.twig
@@ -0,0 +1,61 @@
+            {# Display operation links if the theme is compatible. #}
+            {% if not theme.incompatible %}
+              {{ theme.operations }}
+            {% else %}
+              <div class="incompatible">{{ theme.incompatible }}</div>
+            {% endif %}

The double negatives are hard to read in code. Just swap the blocks and remove 'not'.

Otherwise I like these changes alot:)

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Novice

Hehe, agreed! It felt a bit wrong.

Tagging as a good task for a new contributor to make the change suggested in #20 and post a new patch and interdiff.

jamesrutherford’s picture

Status: Needs work » Needs review
StatusFileSize
new9.36 KB
new818 bytes

Thanks for identifying an easy new contributor task! Here is the new patch and interdiff. Setting to needs review.

star-szr’s picture

Issue tags: -Novice

Changes look good, thanks @jamesrutherford!

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new25.84 KB
new25.57 KB
new26.82 KB
new26.17 KB
new334.94 KB

Back to RTBC, did another markup review and it's looking good. Attached are the before and after markup and the normalized versions for the screenshot and easier review.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Shoot. Really sorry to say, but this one no longer applies. Can we get a quick re-roll?

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community
StatusFileSize
new9.32 KB

Quick re-roll as requested:)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Actually, sorry.. #22 applied just fine with patch -p1.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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