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

Render all Nunjucks before Markdown runs #2565

Merged
merged 1 commit into from
Jan 31, 2023
Merged

Conversation

colinrotherham
Copy link
Contributor

This PR lays the groundwork to fix a bug lurking in our Metalsmith build

Notice how we sometimes render code blocks via Nunjucks:

<pre><code>{% raw %}
{% block skipLink %}
  {{ govukSkipLink({ text: "custom text" }) }}
{% endblock %}{% endraw %}</code></pre>

Or using a global + opt-in syntax highlighting filter:

{{- getNunjucksCode(examplePath) | highlight('js') | safe -}}

But sometimes rendering via Markdown with automatic syntax highlighting:

```javascript
{% raw %}
{% block header %}
  {{ govukFooter({ navigation: [] }) }}
{% endblock %}
{% endraw %}
```

This "works" but stray line breaks can trigger a double-highlight or double-render (code escaped twice) with both render stages (Nunjucks/Markdown) each calling into highlight.js

After this PR is merged we:

  1. Compile Nunjucks 1st using @metalsmith/in-place
  2. Compile Markdown 2nd using @metalsmith/markdown

Allows Nunjucks to render first any HTML that’s incompatible with Markdown
@colinrotherham colinrotherham added 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) Frontend squad labels Jan 30, 2023
@colinrotherham colinrotherham added this to Needs review 🔍 in Design System Sprint Board via automation Jan 30, 2023
@netlify
Copy link

netlify bot commented Jan 30, 2023

You can preview this change here:

Name Link
🔨 Latest commit 9a330aa
🔍 Latest deploy log https://app.netlify.com/sites/govuk-design-system-preview/deploys/63d81fea2bb3aa00074319a8
😎 Deploy Preview https://deploy-preview-2565--govuk-design-system-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@colinrotherham colinrotherham changed the title Render Nunjucks before Markdown Render all Nunjucks before Markdown runs Jan 30, 2023
@domoscargin
Copy link
Contributor

This looks good in principle.

I wonder if it's worth sticking a beautify step in between the Nunjucks and the Markdown.

It shouldn't affect displayed code blocks at all, but should hopefully limit the future diff between updates if somebody's made changes to a Nunjucks template but not worried about whitespace control.

I'm also wondering (but way out of scope for this PR) whether we should run a diff between the main branch's compiled code and PRs, sorta like the dist diff check on govuk-frontend: https://github.com/alphagov/govuk-frontend/blob/main/.github/workflows/diff-change-to-dist.yaml

Just feels like we don't have any easy way of checking diff except for manually doing it locally. I've popped in a possible thing to validate the compiled HTML, but that won't necessarily mean it doesn't hideously break something.

I'm now just using this comment as a sounding board! Whaddya think?

@colinrotherham
Copy link
Contributor Author

@domoscargin Brill. Thanks for looking at this one too:

Have you noticed anything else?

I'm a bit aware that a HTML beautifier might hide potential whitespace issues, had a bit of hesitance to keep the max_preserve_newlines: 0 config versus getting better at our HTML output upstream in govuk-frontend

Knowing it's an issue, I've added this PR too:

👆 Hopefully we can be fully happy in one repo (govuk-frontend) then simply trust the output in this one?

@domoscargin
Copy link
Contributor

domoscargin commented Jan 31, 2023

@domoscargin Brill. Thanks for looking at this one too:

Have you noticed anything else?

I'm a bit aware that a HTML beautifier might hide potential whitespace issues, had a bit of hesitance to keep the max_preserve_newlines: 0 config versus getting better at our HTML output upstream in govuk-frontend

Knowing it's an issue, I've added this PR too:

👆 Hopefully we can be fully happy in one repo (govuk-frontend) then simply trust the output in this one?

I was thinking more of the non-code block Design System website specific stuff (the example macro is a big one), since the majority of code examples are done through the getHtmlCode function, which beautifies 'em anyway. I suppose kinda difficult to separate the two.

In terms of slight annoyances now, the cookie banner is also doing slightly eye-twitchy things with this code

<         <div class="govuk-cookie-banner__content">
<   <p class="govuk-body">We’d like to use analytics cookies so we can understand how you use the Design System and make improvements.</p>
---
>         <div class="govuk-cookie-banner__content">  <p class="govuk-body">We’d like to use analytics cookies so we can understand how you use the Design System and make improvements.</p>

But wrestling with Nunjucks whitespace control proved too annoying for me last night, and it's not really an issue, just irritating.

Copy link
Contributor

@domoscargin domoscargin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks!

I've added an issue for doing something with the compiled diff: #2574

But this is good to go.

@colinrotherham colinrotherham merged commit df48bd7 into main Jan 31, 2023
Design System Sprint Board automation moved this from Needs review 🔍 to Done 🏁 Jan 31, 2023
@colinrotherham colinrotherham deleted the markdown-render-order branch January 31, 2023 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working the way it should (including incorrect wording in documentation)
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants