v10 update #43
v10 update #43
Conversation
Now that nhsuk-frontend publishes its fixtures, we can test for exact compatability of the html output.
When trim_blocks and lstrip_blocks is enabled (the configuration we want) Jinja overzealously removes whitespace due to these inline comment blocks. Override this for consistency with the nhsuk-frontend fixtures. See https://jinja.palletsprojects.com/en/stable/templates/#whitespace-control
We need to use the namespace workaround due to Jinja's stupid scope rules.
Improvement ported from upstream. This is useful for linking error summaries to the field with the error. In the case of radios or checkboxes, the first radio or checkbox should be focused when you click on the error.
null is not a valid literal so we should be using none instead.
this is now supposed to be handled inside textarea itself
nhsuk_frontend_jinja/templates/components/breadcrumb/template.jinja
Outdated
Show resolved
Hide resolved
| {% endif %} | ||
| {% endfor %} | ||
| {% if params.href %} | ||
| <li class="nhsuk-breadcrumb__item"><a class="nhsuk-breadcrumb__link" href="{{ params.href }}">{{ params.text }}</a></li> |
There was a problem hiding this comment.
Sorry, we've got a missed class change here
| <li class="nhsuk-breadcrumb__item"><a class="nhsuk-breadcrumb__link" href="{{ params.href }}">{{ params.text }}</a></li> | |
| <li class="nhsuk-breadcrumb__list-item"> | |
| <a class="nhsuk-breadcrumb__link" href="{{ params.href }}">{{ params.text}}</a> | |
| </li> |
| @@ -94,7 +94,7 @@ | |||
| <div class="nhsuk-form-group {%- if params.errorMessage %} nhsuk-form-group--error{% endif %} {%- if params.formGroup.classes %} {{ params.formGroup.classes }}{% endif %}"> | |||
There was a problem hiding this comment.
Missed the form group attributes here
Same in date input, but maybe others?
| <div class="nhsuk-form-group {%- if params.errorMessage %} nhsuk-form-group--error{% endif %} {%- if params.formGroup.classes %} {{ params.formGroup.classes }}{% endif %}"> | |
| <div class="nhsuk-form-group {%- if params.errorMessage %} nhsuk-form-group--error{% endif %} {%- if params.formGroup.classes %} {{ params.formGroup.classes }}{% endif %}" | |
| {{- nhsukAttributes(params.formGroup.attributes) }}> |
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| {% set headingLevel = params.headingLevel if params.headingLevel else 1 -%} | |||
|
|
|||
| <section class="nhsuk-hero | |||
| <section {%- if params.id %} id="{{ params.id }}"{% endif %} class="nhsuk-hero | |||
There was a problem hiding this comment.
Sorry for the wrong line number
We added the containerClasses param in nhsuk/nhsuk-frontend#1412
Affects the hero, header and footer
- <div class="nhsuk-width-container{% if not params.imageURL %} nhsuk-hero--border{% endif %}">
+ <div class="nhsuk-width-container
+ {%- if params.containerClasses %} {{ params.containerClasses }}{% endif %}
+ {%- if not params.imageURL %} nhsuk-hero--border{% endif %}">| {%- endif -%} | ||
|
|
||
| <div {%- if params.id %} id="{{ params.id }}"{% endif %} class="nhsuk-notification-banner {%- if typeClass %} {{ typeClass }}{% endif %}{% if params.classes %} {{ params.classes }}{% endif %}" role="{{ role }}" aria-labelledby="{{ params.titleId | default("nhsuk-notification-banner-title", true) }}" data-module="nhsuk-notification-banner" | ||
| {%- if params.disableAutoFocus is not undefined %} data-disable-auto-focus="{{ params.disableAutoFocus | lower() }}"{% endif %} |
There was a problem hiding this comment.
Thanks for adding | lower()
Do any other boolean params need it?
There was a problem hiding this comment.
Yes I suspect they do, so far I've only added where they break the tests, but anywhere we coerce booleans to strings is a problem.
I will have a quick look but if some bugs like this slip through it's probably not a big deal, as we can workaround it in application code by passing strings instead of boolean.
There was a problem hiding this comment.
I suppose it's a necessity of the Jinja port?
We should add it everything really
There was a problem hiding this comment.
Yeah, because str(True) == 'True' in python
There was a problem hiding this comment.
looked into this and the only one I found that was rendering the title case was params.spellcheck, which I've now addressed.
I've also amended the attributes macro to lowercase if you pass in an attribute like {"conditional": true, "value", false}.
I also checked use of aria-expanded and aria-hidden, which are all fine, and there are some other attributes like autocomplete/autocapitalize but they use on/off rather true/false so no problem there.
| @@ -90,7 +90,7 @@ | |||
| <div class="nhsuk-form-group {%- if params.errorMessage %} nhsuk-form-group--error{% endif %} {%- if params.formGroup.classes %} {{ params.formGroup.classes }}{% endif %}"> | |||
There was a problem hiding this comment.
Another place we need to support form group attributes
| <div class="nhsuk-form-group {%- if params.errorMessage %} nhsuk-form-group--error{% endif %} {%- if params.formGroup.classes %} {{ params.formGroup.classes }}{% endif %}"> | |
| <div class="nhsuk-form-group {%- if params.errorMessage %} nhsuk-form-group--error{% endif %} {%- if params.formGroup.classes %} {{ params.formGroup.classes }}{% endif %}" | |
| {{- nhsukAttributes(params.formGroup.attributes) }}> |
| <div class="nhsuk-form-group | ||
| {%- if params.errorMessage %} nhsuk-form-group--error{% endif %}"> |
There was a problem hiding this comment.
Form group classes and attributes here
| <div class="nhsuk-form-group | |
| {%- if params.errorMessage %} nhsuk-form-group--error{% endif %}"> | |
| <div class="nhsuk-form-group {%- if params.errorMessage %} nhsuk-form-group--error{% endif %} {%- if params.formGroup.classes %} {{ params.formGroup.classes }}{% endif %}" | |
| {{- nhsukAttributes(params.formGroup.attributes) }}> |
| @@ -8,7 +7,7 @@ | |||
| <h{{ headingLevel }} class="nhsuk-table__heading-tab">{{ params.heading | safe }}</h{{ headingLevel }}> | |||
| {%- endif %} | |||
| {%- endif %} | |||
| <table class="nhsuk-table | |||
| <table {%- if params.id %} id="{{ params.id }}"{% endif %} class="nhsuk-table | |||
There was a problem hiding this comment.
Sorry this one is for line 52
{% set commonContents %}
- {%- if params.responsive -%}
+ {%- if params.responsive and cell.header -%}| {% set describedBy = "" %} | ||
| {%- set describedBy = params.describedBy if params.describedBy else "" -%} | ||
| {%- set id = params.id if params.id else params.name -%} | ||
|
|
||
| <div class="nhsuk-form-group {%- if params.errorMessage %} nhsuk-form-group--error{% endif %} {%- if params.formGroup.classes %} {{ params.formGroup.classes }}{% endif %}"> |
There was a problem hiding this comment.
Another form group attributes
| <div class="nhsuk-form-group {%- if params.errorMessage %} nhsuk-form-group--error{% endif %} {%- if params.formGroup.classes %} {{ params.formGroup.classes }}{% endif %}"> | |
| <div class="nhsuk-form-group {%- if params.errorMessage %} nhsuk-form-group--error{% endif %} {%- if params.formGroup.classes %} {{ params.formGroup.classes }}{% endif %}" | |
| {{- nhsukAttributes(params.formGroup.attributes) }}> |
nhsuk_frontend_jinja/templates/components/warning-callout/README.md
Outdated
Show resolved
Hide resolved
This macro is derived from govuk-frontend and doesn't do anything unique to our design system. So we can use this existing version that already works with Jinja. I previously rewrote this macro to avoid the intermediate attributesHtml variable instead of using the namespace() workaround. This was because of scoping issues in jinja preventing outer variables from being modified in a loop. However, it's simpler to maintain if we use the namespace() workaround as it keeps the code closely matching the upstream version. This also gets rid of the weak equality checks which were a bit dodgy. Initially I wanted to avoid using `is` and other syntax unsupported by nunjucks, but this had no real benefit, and diverging from the original macro makes maintenence harder.
This ensures the behave is the same as the nunjucks templates when a boolean value is passed.
|
This ports over all the changes from the latest nhsuk-frontend-jinja v10 release candidate, and updates the tests.
Note: Sonar flagged that our tests don't set autoescape. However I think there are some templates that assume this is off, see https://nhsd-jira.digital.nhs.uk/browse/DTOSS-9978
It needs fixing but not as part of this update.