Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review use indent in our templates #3211

Closed
3 tasks
owenatgov opened this issue Jan 26, 2023 · 9 comments · Fixed by #4670, #4674, #4673, #4672 or #4671
Closed
3 tasks

Review use indent in our templates #3211

owenatgov opened this issue Jan 26, 2023 · 9 comments · Fixed by #4670, #4674, #4673, #4672 or #4671
Assignees
Labels

Comments

@owenatgov
Copy link
Contributor

What

We use indent across our component templates. Do we still need to do this?

Why

The primary intent of using indent in our context is to make it look tidy when a developer is viewing the source for a page with our components on. There's a question now about how valuable this is based on the reckon that most devs will use dev tools over view source and that we pretty print our HTML.

We should investigate if this is still valuable. If so, we should see if there are things we can do to improve and/or document our use of it. If not, we should remove it.

Notes

Places we use indent

In many of the below examples there are more than one use of indent so look at the whole file as well as the referenced line.

Extra things to think about

  • Could we add something to our load process to auto-indent our HTML?
  • Are there other benefits of indent other than for view source?

Who needs to work on this

Developers

Done when

  • Confirm why we use indent
  • Decide if we should continue using indent or not
  • Consider alternatives to using indent/more efficient ways to leverage indent
@colinrotherham
Copy link
Contributor

colinrotherham commented Nov 9, 2023

Turns out that indent() etc is vital to indent included HTML other than the first line

I've added indent() where necessary and formatted all our Nunjucks HTML in:

@colinrotherham colinrotherham linked a pull request Nov 9, 2023 that will close this issue
@owenatgov
Copy link
Contributor Author

I'm in the process of reviewing that PR but I don't fully grasp how indent() is vital. Could you elaborate please?

@colinrotherham
Copy link
Contributor

Yeah course, I should say it's vital to "carry the indent level" from the one macro to the next for tidiness sake only

I'll add some examples below using this little unindented macro:

{% macro govukExample() %}
<article>
  <h2>Example</h2>
  <p>Hello Owen</p>
</article>
{% endmacro %}

Without indent

See how the macro contents isn't indented at all?

It appears to indent the macro's 1st HTML line, but that's from the 2 spaces before {{ govukExample() }}

Input

<section>
  <h1>Demo</h1>
  {{ govukExample() }}
</section>

Output

<section>
  <h1>Demo</h1>
  <article>
  <h2>Example</h2>
  <p>Hello Owen</p>
</article>

</section>

Without indent, nested

As above, only the macro's 1st HTML line is indented by the spaces before {{ govukExample() }}

But none of the macro's subsequent HTML lines are indented at all

Input

<div class="govuk-grid-row">
  <div class="govuk-grid-column">
    <section>
      <h1>Demo</h1>
      {{ govukExample() }}
    </section>
  </div>
</div>

Output

<div class="govuk-grid-row">
  <div class="govuk-grid-column">
    <section>
      <h1>Demo</h1>
      <article>
  <h2>Example</h2>
  <p>Hello Owen</p>
</article>

    </section>
  </div>
</div>

With indent

Notice now how indent(2) adds 2 spaces to the macro's 2nd, 3rd and 4th HTML lines too?

Input

<section>
  <h1>Demo</h1>
  {{ govukExample() | trim | indent(2) }}
</section>

Output

<section>
  <h1>Demo</h1>
  <article>
    <h2>Example</h2>
    <p>Hello Owen</p>
  </article>
</section>

With indent, nested

With a nested example we now begin at 6 spaces (not 2) so we need indent(2) indent(6)

But notice how indent(6) still adds 6 spaces to each subsequent macro HTML line?

Input

<div class="govuk-grid-row">
  <div class="govuk-grid-column">
    <section>
      <h1>Demo</h1>
      {{ govukExample() | trim | indent(6) }}
    </section>
  </div>
</div>

Output

<div class="govuk-grid-row">
  <div class="govuk-grid-column">
    <section>
      <h1>Demo</h1>
      <article>
        <h2>Example</h2>
        <p>Hello Owen</p>
      </article>
    </section>
  </div>
</div>

@colinrotherham
Copy link
Contributor

Just a quick variation to the above too

Lots of our formatting issues were with params.html which also needs indenting

For example, take this Nunjucks macro instead:

{% macro govukExample(html) %}
<article>
  {{ html | trim | safe | indent(2) }}
</article>
{% endmacro %}

With indent, nested html param

Taking the macro above, notice how we indent both the macro contents and the {% set html %} param?

  1. Nunjucks view applies indent(2) to govukExample() affecting <article> contents
  2. Nunjucks macro applies indent(2) to the {% set html %} param contents

Input

{% set html %}
<h1>What about me?<h1>
<p>Indents can be cumulative<p>
{% endset -%}

<section>
  <h1>Demo</h1>
  {{ govukExample(html) | trim | indent(2) }}
</section>

Output

<section>
  <h1>Demo</h1>
  <article>
    <h1>What about me?<h1>
    <p>Indents can be cumulative<p>
  </article>
</section>

@owenatgov
Copy link
Contributor Author

Cheers Colin that makes sense!

@colinrotherham colinrotherham moved this from Needs review 🔍 to In progress 📝 in GOV.UK Design System cycle board Nov 27, 2023
@colinrotherham
Copy link
Contributor

I've added some suggestions to Accordion, Breadcrumbs and Button for @owenatgov to review

@colinrotherham
Copy link
Contributor

colinrotherham commented Feb 13, 2024

Thanks for all these reviews @romaricpascal

Thought I'd got them all but I'd not split out Task list or Tabs so will reopen

You'll see some more PRs coming through shortly

@colinrotherham
Copy link
Contributor

@owenatgov All done ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment