Problem/Motivation

Bartik theme templates are incorrectly indented making readability poor. The templates need to be structured to follow best coding practises.

For eg.

<div id="messages"><div class="section clearfix">

Should be:

<div id="messages">
  <div class="section clearfix">

Follow up from #1179764: Convert Bartik to HTML5

Proposed resolution

Clean up the templates. Making sure proper indentation, the right use of line breaks and blank lines etc are used so the templates are clear to read.

Documentation on Twig coding standards can be found here:
Drupal Twig coding standards : https://www.drupal.org/node/1823416
Twig coding standards: http://twig.sensiolabs.org/doc/coding_standards.html

Check out the #drupal-twig IRC channel if you need to ask questions.

Also look at other Twig template files in module files in Core for real examples.

Remaining tasks

Write a patch.
Check the patch passes with testbot.
Check the patch applies to Core.
Check nothing is broken on Bartik visually.
Add screenshots if patch is good or bad.
Review patch to check coding standards.

User interface changes

n/a

API changes

n/a

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because coding standards
Unfrozen changes Unfrozen because it only changes code formatting, it doesn't change mark up
CommentFileSizeAuthor
#34 drupal-clean_bartik_html_formatting-1903048-34.patch17.4 KBemma.maria
#34 interdiff.txt4.02 KBemma.maria
#32 interdiff.txt8.49 KBlauriii
#32 drupal-clean_bartik_html_formatting-1903048-32.patch17.46 KBlauriii
#30 interdiff-1903048-28-30.txt7.16 KBDickJohnson
#30 drupal-clean_bartik_html_formatting-1903048-30.patch18.42 KBDickJohnson
#28 interdiff-1903048-17-28.txt14.93 KBDickJohnson
#28 drupal-clean_bartik_html_formatting-1903048-28.patch15.37 KBDickJohnson
#17 drupal-clean_bartik_html_formatting-1903048-17.patch11.6 KBGemVinny
#13 drupal-clean_bartik_html_formatting-1903048-13.patch16.12 KBemma.maria
#6 drupal-clean_bartik_html_formatting-1903048-6.patch14.53 KBAnonymous (not verified)
#1 drupal-clean_bartik_html_formatting-1903048-1.patch21.04 KBmbrett5062
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mbrett5062’s picture

mbrett5062 CreditAttribution: mbrett5062 commented
Assigned: Unassigned » mbrett5062
Status: Active » Needs review
FileSize
drupal-clean_bartik_html_formatting-1903048-1.patch21.04 KB

Have cleaned up the indentation and general formatting of Bartik's template files. This makes for better readability and as an added bonus, code folding now works as expected. Hopefully this will help speed up any future work on these templates, and aid in new user understanding of the template structure.

mbrett5062’s picture

mbrett5062 CreditAttribution: mbrett5062 commented
Assigned: mbrett5062 » Unassigned

OK that's good to go as far as I am concerned. Just need someone to review. Unassinging myself for now, to hopefully attract a reviewer.

mbrett5062’s picture

mbrett5062 CreditAttribution: mbrett5062 commented
Issue summary: View changes

Updated issue summary.

enhdless’s picture

enhdless CreditAttribution: enhdless commented
Issue summary: View changes
Status: Needs review » Needs work

Nice patch, but the theme files have changed, so a new patch is needed.

error: core/themes/bartik/templates/comment-wrapper.tpl.php: No such file or directory
error: core/themes/bartik/templates/comment.tpl.php: No such file or directory
error: core/themes/bartik/templates/maintenance-page.tpl.php: No such file or directory
error: core/themes/bartik/templates/node.tpl.php: No such file or directory
error: core/themes/bartik/templates/page.tpl.php: No such file or directory
emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue tags: +CSS, +Coding standards, +frontend, +Needs reroll
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Assigned: Unassigned »
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
drupal-clean_bartik_html_formatting-1903048-6.patch14.53 KB

Here's a new patch. Thanks to @olemedia.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Assigned: » Unassigned
emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Status: Needs review » Needs work

The indentations look great but there are a lot of inconsistent blank lines being used, especially in comment.html.twig and node.html.twig. So I still find the code hard to read.

After discussion at the Frontend table at Drupalcon Austin, we think all blank lines within Twig templates should be avoided or used if really really really needed in a consistent way.

We are waiting on final clarification from the Twig pros for the final verdict.

JacobSanford’s picture

JacobSanford
Primary language English
Location Fredericton 🇨🇦
CreditAttribution: JacobSanford commented
Issue tags: -Needs reroll

Removing 'Needs reroll' tag, looks to have been done in #6. Retesting to be sure.

JacobSanford’s picture

JacobSanford
Primary language English
Location Fredericton 🇨🇦
CreditAttribution: JacobSanford commented
Status: Needs work » Needs review

6: drupal-clean_bartik_html_formatting-1903048-6.patch queued for re-testing.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue tags: +Needs a reroll
emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
FileSize
drupal-clean_bartik_html_formatting-1903048-13.patch16.12 KB

The reroll was ugly as some of the templates have been reorganised and had a lot of things added to them since this issue was last worked on.

Therefore I went through the remaining templates that still needed work today and fixed the indentations in them as well as the inconsistent line spacing discussed in #9.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue tags: -Needs a reroll
sqndr’s picture

sqndr CreditAttribution: sqndr commented
Status: Needs review » Needs work 
+++ b/core/themes/bartik/templates/maintenance-page.html.twig
@@ -8,38 +8,50 @@
+  <div id="page-wrapper">
+    <div id="page">

This template file starts with 2 spaces. I feel like those can be removed as well.

GemVinny’s picture

GemVinny CreditAttribution: GemVinny commented
Assigned: Unassigned » GemVinny
GemVinny’s picture

GemVinny CreditAttribution: GemVinny commented
Assigned: GemVinny » Unassigned
Status: Needs work » Needs review
FileSize
drupal-clean_bartik_html_formatting-1903048-17.patch11.6 KB

I have removed the 2 spaces at the start of the template. I have also cleaned up some other formatting in core/themes/bartik/templates/comment.html.twig

Status: Needs review » Needs work

The last submitted patch, 17: drupal-clean_bartik_html_formatting-1903048-17.patch, failed testing.

Status: Needs work » Needs review

emma.maria queued 17: drupal-clean_bartik_html_formatting-1903048-17.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 17: drupal-clean_bartik_html_formatting-1903048-17.patch, failed testing.

Status: Needs work » Needs review

emma.maria queued 17: drupal-clean_bartik_html_formatting-1903048-17.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 17: drupal-clean_bartik_html_formatting-1903048-17.patch, failed testing.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Assigned: Unassigned » emma.maria

The patch applies but it breaks the UI and testbot is failing.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Assigned: emma.maria » Unassigned
GemVinny’s picture

GemVinny CreditAttribution: GemVinny commented
Assigned: Unassigned » GemVinny
emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue summary: View changes
Issue tags: +Novice
DickJohnson’s picture

DickJohnson CreditAttribution: DickJohnson commented
Assigned: GemVinny » DickJohnson
DickJohnson’s picture

DickJohnson CreditAttribution: DickJohnson commented
Status: Needs work » Needs review
FileSize
drupal-clean_bartik_html_formatting-1903048-28.patch15.37 KB
interdiff-1903048-17-28.txt14.93 KB

Rerolled and fixed stuff on four different template-files. Don't think it's ready but patching changes to get it to either myself on later time or to someone else.

DickJohnson’s picture

DickJohnson CreditAttribution: DickJohnson commented
Assigned: DickJohnson » Unassigned
DickJohnson’s picture

DickJohnson CreditAttribution: DickJohnson commented
Assigned: Unassigned » DickJohnson
FileSize
drupal-clean_bartik_html_formatting-1903048-30.patch18.42 KB
interdiff-1903048-28-30.txt7.16 KB

New patch around.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Assigned: DickJohnson » Unassigned

@DickJohnson Please unassign when you are ready for an issue to be reviewed :)

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
FileSize
drupal-clean_bartik_html_formatting-1903048-32.patch17.46 KB
interdiff.txt8.49 KB

There was some coding standard breaking code still

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented

Reviewing this now.
I also created a follow up issue to get all of the <!-- /something--> comments out of the templates. They are not needed.
#2395825: Remove closing tag comments in template files.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
FileSize
interdiff.txt4.02 KB
drupal-clean_bartik_html_formatting-1903048-34.patch17.4 KB

I noticed a few more indentation errors, mostly in page.html.twig.
See interdiff and new patch.

We need at least one more set of fresh eyes on this to be able to set it to RTBC.

Status: Needs review » Needs work

The last submitted patch, 34: drupal-clean_bartik_html_formatting-1903048-34.patch, failed testing.

Status: Needs work » Needs review

basic queued 34: drupal-clean_bartik_html_formatting-1903048-34.patch for re-testing.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

This looks like a nice cleanup. Thank you @emma.maria et al!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: drupal-clean_bartik_html_formatting-1903048-34.patch, failed testing.

Status: Needs work » Needs review

LewisNyman queued 34: drupal-clean_bartik_html_formatting-1903048-34.patch for re-testing.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC. testbot hiccup.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Issue summary: View changes

Added beta evaluation

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: drupal-clean_bartik_html_formatting-1903048-34.patch, failed testing.

DickJohnson’s picture

DickJohnson CreditAttribution: DickJohnson commented
Status: Needs work » Reviewed & tested by the community

Patch applies and works for me so queueing and back to RTBC.

DickJohnson queued 34: drupal-clean_bartik_html_formatting-1903048-34.patch for re-testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Ahhh, that's nice on the eyes. :)

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 847a50a on 8.0.x 
    Issue #1903048 by DickJohnson, emma.maria, lauriii, mbrett5062,...
emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented

Thanks @webchick :)

Status: Fixed » Closed (fixed)

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