Review and fix Nunjucks HTML indentation #4448

colinrotherham · 2023-11-09T14:38:58Z

This PR updates all Nunjucks templates with correctly indented HTML to close:

Review use indent in our templates #3211

With two caveats:

Textarea (and Character count) skips some indents to preserve <textarea> content
Details component skips some indents to prevent extra spaces in code previews

Before

Checkboxes (without js-beautify formatter) from PR #4447
https://govuk-frontend-pr-4447.herokuapp.com/components/checkboxes

<div class="govuk-form-group">
  <div class="govuk-checkboxes"    data-module="govuk-checkboxes">
          <div class="govuk-checkboxes__item">
            <input class="govuk-checkboxes__input" id="nationality" name="nationality" type="checkbox" value="british">
            <label class="govuk-label govuk-checkboxes__label" for="nationality">
        British
      </label>
          </div>
          <div class="govuk-checkboxes__item">
            <input class="govuk-checkboxes__input" id="nationality-2" name="nationality" type="checkbox" value="irish">
            <label class="govuk-label govuk-checkboxes__label" for="nationality-2">
        Irish
      </label>
          </div>
          <div class="govuk-checkboxes__item">
            <input class="govuk-checkboxes__input" id="nationality-3" name="nationality" type="checkbox" value="other">
            <label class="govuk-label govuk-checkboxes__label" for="nationality-3">
        Citizen of another country
      </label>
          </div>
  </div>
</div>

After

Checkboxes (without js-beautify formatter) from this PR
https://govuk-frontend-pr-4448.herokuapp.com/components/checkboxes

<div class="govuk-form-group">
  <div class="govuk-checkboxes" data-module="govuk-checkboxes">
    <div class="govuk-checkboxes__item">
      <input class="govuk-checkboxes__input" id="nationality" name="nationality" type="checkbox" value="british">
      <label class="govuk-label govuk-checkboxes__label" for="nationality">
        British
      </label>
    </div>
    <div class="govuk-checkboxes__item">
      <input class="govuk-checkboxes__input" id="nationality-2" name="nationality" type="checkbox" value="irish">
      <label class="govuk-label govuk-checkboxes__label" for="nationality-2">
        Irish
      </label>
    </div>
    <div class="govuk-checkboxes__item">
      <input class="govuk-checkboxes__input" id="nationality-3" name="nationality" type="checkbox" value="other">
      <label class="govuk-label govuk-checkboxes__label" for="nationality-3">
        Citizen of another country
      </label>
    </div>
  </div>
</div>

github-actions · 2023-11-09T14:42:23Z

📋 Stats

File sizes

File	Size
dist/govuk-frontend-development.min.css	112.45 KiB
dist/govuk-frontend-development.min.js	38.58 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js	78.74 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs	73.99 KiB
packages/govuk-frontend/dist/govuk/all.mjs	3.86 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs	359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.css	112.44 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js	38.57 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs	5.38 KiB

Modules

File	Size
all.mjs	70.32 KiB
components/accordion/accordion.mjs	21.67 KiB
components/button/button.mjs	4.7 KiB
components/character-count/character-count.mjs	21.24 KiB
components/checkboxes/checkboxes.mjs	5.83 KiB
components/error-summary/error-summary.mjs	6.57 KiB
components/exit-this-page/exit-this-page.mjs	16.08 KiB
components/header/header.mjs	4.46 KiB
components/notification-banner/notification-banner.mjs	4.93 KiB
components/radios/radios.mjs	4.83 KiB
components/skip-link/skip-link.mjs	4.39 KiB
components/tabs/tabs.mjs	10.16 KiB

View stats and visualisations on the review app

Action run for c6eca9c

colinrotherham · 2023-11-09T14:47:04Z

Here's a diff of the HTML changes for all examples (bit big and repetitive) since #4416

html.diff.zip

owenatgov · 2023-11-17T11:00:08Z

I made a start on review and had began commenting on instances where the indent isn't respected for nunjucks functions like if and for blocks before I figured out that this changes enforces respect of html indentation but not nunjucks indentation. I'm feeling resistant to this as for me, it's going to make interpreting nunjucks code a lot trickier.

I there something we can do to clean up this extra indentation on render?

colinrotherham · 2023-11-17T12:56:52Z

Thanks @owenatgov

You mean how I've moved away from indenting {% for %} and {% if %} for the sake of HTML?

Yeah I'd love to have an alternative too, it's definitely a compromise

I'll show a few of the options I tried here

Indenting both Nunjucks tags and HTML

This is what we mostly do currently, but indenting each Nunjucks tag level effectively double-indents the HTML

Input

{% if items | length %}
  <ul>
    {% for item in items %}
      <li>
        {% if item.active %}
          <a class="active">{{ item.text }}</a>
        {% else %}
          <a>{{ item.text }}</a>
        {% endif %}
      </li>
    {% endfor %}
  </ul>
{% endif %}

Output

The wrapping {% if items | length %} leaves the HTML indented by 2-spaces unnecessarily

Plus each Nunjucks tag level adds another 2-space indent

  <ul>
      <li>
          <a>One</a>
      </li>
      <li>
          <a class="active">Two</a>
      </li>
  </ul>

Indenting Nunjucks tags only

This is what I've tried to do as compromise, i.e. taking Nunjucks tags back an indent level alongside the HTML

You'll notice in lots of places though that we nest so deeply it's not possible

Input

{% if items | length %}
<ul>
  {% for item in items %}
  <li>
    {% if item.active %}
    <a class="active">{{ item.text }}</a>
    {% else %}
    <a>{{ item.text }}</a>
    {% endif %}
  </li>
  {% endfor %}
</ul>
{% endif %}

Output

HTML output is correctly 2-space indented

<ul>
  <li>
    <a>One</a>
  </li>
  <li>
    <a class="active">Two</a>
  </li>
</ul>

Indenting Nunjucks tags with indented macros

Alternatively, using the Summary list approach (with calls to _actionLink() and _summaryCard()) there's a third solution using macros but we'd have to strictly control indentation at the point every macro was called

I definitely think this can get too complicated but can be used sparingly

When nesting too deeply we'd then need clever combinations of {{- and trim to discard the indent level, followed by indent(2, true) (using true to indent the first line), to restore the correct level 😣

Input

{#- Render `<ul>` elements -#}
{%- macro list(items) %}
  <ul>
    {% for item in items -%}
      {{- listItem(item) -}}
    {% endfor -%}
  </ul>
{% endmacro -%}

{#- Render `<li>` elements -#}
{%- macro listItem(item) %}
  <li>
    {{ listLink(item) | trim }}
  </li>
{% endmacro -%}

{#- Render `<a>` elements -#}
{%- macro listLink(item) -%}
  {% if item.active %}
    <a class="active">{{ item.text }}</a>
  {% else %}
    <a>{{ item.text }}</a>
  {% endif %}
{%- endmacro -%}

{% if items | length %}
  {{- list(items) | trim -}}
{% endif %}

Output

HTML output is correctly 2-space indented

<ul>
  <li>
    <a>One</a>
  </li>
  <li>
    <a class="active">Two</a>
  </li>
</ul>

colinrotherham · 2023-11-17T13:05:00Z

I there something we can do to clean up this extra indentation on render?

Yeah we currently use js-beautify to format the Review app code examples

But it means we miss HTML formatting issues, because we see the formatted version not the Nunjucks output

I've turned the formatter off via c9f891b in this PR (see preview)

colinrotherham · 2023-11-23T11:04:00Z

We had a dev-catch up this morning

We should still clean up the HTML but restore Nunjucks nesting for readability

I'll give this PR an update and move it back into draft for now

owenatgov

Thanks for this Colin. I've left a bunch of "indent query"'s Where I wanna understand what the impact of retaining the nunjucks indent is to the HTML, if it becomes too wild etc. I've been a bit lazy in how I've identified these lacks of indents because of uncertainties in the problem space itself.

An aside: I'm personally warming to the idea of us authoring our HTML in a more considered way, however I think if our nunjucks indentation is wild enough in a given template that it'll cause problems for our html authoring then we need to:

find ways to re-structure that given template to reduce the amount of Christmas tree code
communicate somewhere other than in this PR why we're ignoring nunjucks indentation

Another aside: how difficult would it be do you reckon to write something that automatically re-authored our nunjucks in response to the html output? This is very much spitballing but I wonder if we could set rules for how much html deviates from ideal authorship/indentation and then a script would fix those instances for us. I'd love it if nunjucks automated this at the build phase but it looks like that's just not something nunjucks can do.

packages/govuk-frontend-review/src/common/nunjucks/globals/get-html-code.mjs

owenatgov · 2023-11-24T11:31:14Z

packages/govuk-frontend/src/govuk/components/accordion/template.njk

-            <p class="govuk-body">{{ item.content.text }}</p>
-          {% endif %}
-        </div>
+  {% if item %}


Could we indent the nunjucks here or does it make the html too gross?

@owenatgov Instead of indenting more (to add extra spaces to the HTML) how about un-indenting?

Just pushed, here's Accordion:

https://github.com/alphagov/govuk-frontend/compare/7a8beb4dd425acf91c07389b2d9e847d15e91966..196ba655eab63dc9ade9178eff00f63afcf0542c#diff-0b0d5366e0718a4d3960cf2f8a280eede3edf141491d8d99c5dc9cdc39a3f4e5

owenatgov · 2023-11-24T11:31:59Z

packages/govuk-frontend/src/govuk/components/breadcrumbs/template.njk

@@ -9,15 +9,19 @@
  {% set classNames = classNames + " govuk-breadcrumbs--collapse-on-mobile" %}
 {% endif -%}

-<div class="{{classNames}}"{% for attribute, value in params.attributes %} {{attribute}}="{{value}}"{% endfor %}>
+<div class="{{ classNames }}" {%- for attribute, value in params.attributes %} {{ attribute }}="{{ value }}"{% endfor %}>
  <ol class="govuk-breadcrumbs__list">
  {% for item in params.items %}
    {% if item.href %}
    <li class="govuk-breadcrumbs__list-item">


Indent query here and on line 22

@owenatgov Same with this one, could we drop the Nunjucks back a level?

Just pushed, here's Breadcrumbs:

https://github.com/alphagov/govuk-frontend/compare/7a8beb4dd425acf91c07389b2d9e847d15e91966..196ba655eab63dc9ade9178eff00f63afcf0542c#diff-83e9ea311b994f62913033b905299b893bfee1b0daf8c5b0d85b92a0c2995134

owenatgov · 2023-11-24T11:32:32Z

packages/govuk-frontend/src/govuk/components/button/template.njk

-</svg>
-  {% endset %}
-  {% set classNames = classNames + " govuk-button--start" %}
+{% set iconHtml -%}


Indent query

We can either have the start button SVG indented too far, or move it into a macro?

Just pushed, here's Button:

https://github.com/alphagov/govuk-frontend/compare/7a8beb4dd425acf91c07389b2d9e847d15e91966..196ba655eab63dc9ade9178eff00f63afcf0542c#diff-b7e04180b95a23ebfc75404577f0ae0f036dec05b7df981ad9b8dd0b2255e1da

owenatgov · 2023-11-24T11:44:51Z

packages/govuk-frontend/src/govuk/components/checkboxes/template.njk

-            </div>
-          {% endif %}
-        {% endif %}
+  {%- for attribute, value in params.attributes %} {{ attribute }}="{{ value }}"{% endfor %} data-module="govuk-checkboxes">


Indent query. This one looks like it's going to be tricky so I anticipate being pushed back against.

owenatgov · 2023-11-24T12:05:54Z

packages/govuk-frontend/src/govuk/components/summary-list/template.njk

+    </h{{ headingLevel }}>
+    {% endif %}
+    {% if params.actions.items.length %}
+    {% if params.actions.items.length == 1 %}


ident query. I suspect this one is going to massively indent the summary card markup so I'm open to pushback.

owenatgov · 2023-11-24T12:06:27Z

packages/govuk-frontend/src/govuk/components/summary-list/template.njk

-          {% endif %}
-        </div>
+{%- set summaryList %}
+<dl class="govuk-summary-list {%- if params.classes %} {{ params.classes }}{% endif %}" {%- for attribute, value in params.attributes %} {{ attribute }}="{{ value }}"{% endfor %}>


indent query. What's wrong with the minus on the end of the set statement above?

owenatgov · 2023-11-24T12:06:37Z

packages/govuk-frontend/src/govuk/components/summary-list/template.njk

+<dl class="govuk-summary-list {%- if params.classes %} {{ params.classes }}{% endif %}" {%- for attribute, value in params.attributes %} {{ attribute }}="{{ value }}"{% endfor %}>
+{% for row in params.rows %}
+  {% if row %}
+  <div class="govuk-summary-list__row {%- if anyRowHasActions and not row.actions.items %} govuk-summary-list__row--no-actions{% endif %} {%- if row.classes %} {{ row.classes }}{% endif %}">


indent query

owenatgov · 2023-11-24T13:30:36Z

packages/govuk-frontend/src/govuk/components/summary-list/template.njk

+      {{ row.value.html | safe | trim | indent(6) if row.value.html else row.value.text }}
+    </dd>
+    {% if row.actions.items.length %}
+    <dd class="govuk-summary-list__actions {%- if row.actions.classes %} {{ row.actions.classes }}{% endif %}">


indent query, same for lines 66 and 68

owenatgov · 2023-11-24T13:30:53Z

packages/govuk-frontend/src/govuk/components/table/template.njk

-        {% endfor %}
-        </tr>
+  {% for row in params.rows %}
+  {% if row %}


indent query

github-actions · 2023-12-01T15:07:59Z

Other changes to npm package

The diff could not be posted as a comment. You can download it from the workflow artifacts.

Action run for c6eca9c

colinrotherham · 2024-01-26T10:35:06Z

Going to close this one in favour of smaller easier-to-review PRs

RichardBradley · 2024-02-27T12:14:12Z

See #4676 (comment)

I think app supplied html params ought to be excluded from auto-indenting

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4448 November 9, 2023 14:39 Inactive

colinrotherham mentioned this pull request Nov 9, 2023

Review and fix HTML attribute trailing spaces etc #4416

Merged

colinrotherham mentioned this pull request Nov 9, 2023

Review use indent in our templates #3211

Closed

3 tasks

colinrotherham linked an issue Nov 9, 2023 that may be closed by this pull request

Review use indent in our templates #3211

Closed

3 tasks

colinrotherham force-pushed the attributes-trim branch from b14eb6f to dc3bd62 Compare November 15, 2023 19:50

colinrotherham force-pushed the nunjucks-formatting branch from 4ea9027 to c9f891b Compare November 15, 2023 19:51

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4448 November 15, 2023 19:51 Inactive

colinrotherham force-pushed the attributes-trim branch from dc3bd62 to d863a55 Compare November 20, 2023 15:55

Base automatically changed from attributes-trim to main November 20, 2023 16:12

colinrotherham marked this pull request as draft November 23, 2023 11:05

owenatgov reviewed Nov 24, 2023

View reviewed changes

colinrotherham force-pushed the nunjucks-formatting branch from c9f891b to b516760 Compare November 30, 2023 14:20

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4448 November 30, 2023 14:21 Inactive

colinrotherham force-pushed the nunjucks-formatting branch from b516760 to 5ea5cf4 Compare November 30, 2023 15:14

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4448 November 30, 2023 15:14 Inactive

colinrotherham changed the base branch from main to html-diff December 1, 2023 10:40

colinrotherham force-pushed the nunjucks-formatting branch from 5ea5cf4 to 7a8beb4 Compare December 1, 2023 10:40

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4448 December 1, 2023 10:40 Inactive

alphagov deleted a comment from github-actions bot Dec 1, 2023

colinrotherham force-pushed the html-diff branch from 9ed11f6 to 89599fe Compare December 1, 2023 14:38

colinrotherham force-pushed the nunjucks-formatting branch from 7a8beb4 to 196ba65 Compare December 1, 2023 15:05

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4448 December 1, 2023 15:05 Inactive

alphagov deleted a comment from github-actions bot Dec 1, 2023

colinrotherham added 11 commits January 26, 2024 10:34

Indent HTML for Inset text

ea969a2

Indent HTML for Notification banner

05a2f2f

Indent HTML for Pagination

6f6c67f

Indent HTML for Panel

c897c47

Indent HTML for Phase banner

4da1bce

Indent HTML for Radios

7df30d8

Indent HTML for Select

b08c030

Indent HTML for Summary list

4131f4e

Indent HTML for Table

5fc9c8b

Indent HTML for Text input

e03c492

Indent HTML for Textarea

c6eca9c

colinrotherham force-pushed the nunjucks-formatting branch from 35b7a29 to c6eca9c Compare January 26, 2024 10:34

colinrotherham closed this Jan 26, 2024

This was referenced Feb 2, 2024

Fix Nunjucks HTML indentation: Summary list #4716

Merged

Fix Nunjucks HTML indentation: Table #4717

Merged

Fix Nunjucks HTML indentation: Header #4745

Merged

This was referenced Feb 13, 2024

Fix Nunjucks HTML indentation: Tabs #4764

Merged

Fix Nunjucks HTML indentation: Task list, Tag #4767

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review and fix Nunjucks HTML indentation #4448

Review and fix Nunjucks HTML indentation #4448

colinrotherham commented Nov 9, 2023 •

edited

Loading

github-actions bot commented Nov 9, 2023 •

edited

Loading

colinrotherham commented Nov 9, 2023 •

edited

Loading

owenatgov commented Nov 17, 2023

colinrotherham commented Nov 17, 2023 •

edited

Loading

colinrotherham commented Nov 17, 2023

colinrotherham commented Nov 23, 2023 •

edited

Loading

owenatgov left a comment

owenatgov Nov 24, 2023

colinrotherham Dec 1, 2023

owenatgov Nov 24, 2023

colinrotherham Dec 1, 2023

owenatgov Nov 24, 2023

colinrotherham Dec 1, 2023

owenatgov Nov 24, 2023

owenatgov Nov 24, 2023

owenatgov Nov 24, 2023

owenatgov Nov 24, 2023

owenatgov Nov 24, 2023

owenatgov Nov 24, 2023

github-actions bot commented Dec 1, 2023 •

edited

Loading

colinrotherham commented Jan 26, 2024

RichardBradley commented Feb 27, 2024

Review and fix Nunjucks HTML indentation #4448

Review and fix Nunjucks HTML indentation #4448

Conversation

colinrotherham commented Nov 9, 2023 • edited Loading

Before

After

github-actions bot commented Nov 9, 2023 • edited Loading

📋 Stats

File sizes

Modules

colinrotherham commented Nov 9, 2023 • edited Loading

owenatgov commented Nov 17, 2023

colinrotherham commented Nov 17, 2023 • edited Loading

Indenting both Nunjucks tags and HTML

Input

Output

Indenting Nunjucks tags only

Input

Output

Indenting Nunjucks tags with indented macros

Input

Output

colinrotherham commented Nov 17, 2023

colinrotherham commented Nov 23, 2023 • edited Loading

owenatgov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 1, 2023 • edited Loading

Other changes to npm package

colinrotherham commented Jan 26, 2024

RichardBradley commented Feb 27, 2024

colinrotherham commented Nov 9, 2023 •

edited

Loading

github-actions bot commented Nov 9, 2023 •

edited

Loading

colinrotherham commented Nov 9, 2023 •

edited

Loading

colinrotherham commented Nov 17, 2023 •

edited

Loading

colinrotherham commented Nov 23, 2023 •

edited

Loading

github-actions bot commented Dec 1, 2023 •

edited

Loading