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

Format html templates using djlint #1350

Merged
merged 41 commits into from
Apr 26, 2024
Merged

Conversation

petechd
Copy link
Contributor

@petechd petechd commented Apr 9, 2024

What is the context of this PR?

We want to ensure that code is consistent across all our HTML templates in Runner. It will make it easier to maintain code over time, as the code is formatted uniformly and is easier to read and understand.
Files are formatted using djlint, some config was added, DS components are all set to be ignored since djlint cannot cope with their nesting. They were nested manually using a style that improves readability.
Some html tags content in some html templates had to be placed in strings to avoid multiple lines that will cause issues with html rendering in our integration tests. Some inconsistency may be present as this is the first time we try to force uniform formatting in all the html templates.
To run djlint install using npm install or pipenv install and run:
djlint ./templates --reformat --profile=jinja - to format
or
djlint ./templates --profile=jinja - to lint
You can optionally add formatting of JavaScript since we have some tiny snippets but the main JS block in _base.html cannot be formatted since it uses Javascript and combines it with jinja vars.
djlint ./templates --reformat --profile=jinja --format-js

How to review

Some of the whitespace, new line and nesting rules are debatable so check if you agree.
Check djlint configuration doc for possible improvements that can be added. There might be some discrepancy and inconsistencies since this is the first time we introduce uniform formatting to Runner's html files. Error templates files (all in the "error" folder) are the only one that required some content moving/replacing so it's worth double checking if all the sentences are correct now.

Checklist

  • New static content marked up for translation
  • Newly defined schema content included in eq-translations repo

@petechd petechd force-pushed the format-html-templates-in-runner branch from 02d8cd1 to f85d195 Compare April 10, 2024 08:20
@petechd petechd marked this pull request as ready for review April 11, 2024 12:01
Copy link
Contributor

@liamtoozer liamtoozer left a comment

Choose a reason for hiding this comment

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

The changes look good! There are a few blocks that djlint could be switched back on (mentioned in the comments), but I'm not sure if that might litter the files a bit too much with the 'on/off' stuff, or even if it might be overkill? (Confirmed not possible)

Just a quick question too, would we be adding the djlint dependency, readme doc, GitHub Actions etc as part of this ticket?

@petechd
Copy link
Contributor Author

petechd commented Apr 18, 2024

The changes look good! There are a few blocks that djlint could be switched back on (mentioned in the comments), but I'm not sure if that might litter the files a bit too much with the 'on/off' stuff, or even if it might be overkill?

Just a quick question too, would we be adding the djlint dependency, readme doc, GitHub Actions etc as part of this ticket?

Based on the conversations I had with @MebinAbraham my understanding is it is not part of this ticket, this is just to format the files.

Copy link
Contributor

@liamtoozer liamtoozer left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍

@petechd petechd merged commit 5dbd1a5 into main Apr 26, 2024
16 checks passed
@petechd petechd deleted the format-html-templates-in-runner branch April 26, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants