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

Fix Nunjucks default() values when null, false or "" is provided #4327

Merged
merged 4 commits into from
Oct 19, 2023

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Oct 11, 2023

This PR ensures Nunjucks default() handles all possible "falsey" values

It closes #4305 which shows that undefined values fall back to defaults() but not null, false, "" etc

Nunjucks default example

For example, passing redirectUrl: null into Exit this page renders a default submit button ๐Ÿ˜ฎ

{{ govukExitThisPage({
  redirectUrl: null
}) }}

The fallback href value isn't used:

<button type="submit">โ€ฆ</button>

@colinrotherham colinrotherham added ๐Ÿ› bug Something isn't working the way it should (including incorrect wording in documentation) nunjucks labels Oct 11, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4327 October 11, 2023 14:14 Inactive
@github-actions
Copy link

github-actions bot commented Oct 11, 2023

๐Ÿ“‹ Stats

File sizes

File Size
dist/govuk-frontend-4.7.0.min.css 120.66 KiB
dist/govuk-frontend-4.7.0.min.js 51.56 KiB
dist/govuk-frontend-ie8-4.7.0.min.css 81.59 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 74.96 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 70.36 KiB
packages/govuk-frontend/dist/govuk/all.mjs 3.8 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs 359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.css 113.99 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 36.32 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.3 KiB

Modules

File Size
all.mjs 66.68 KiB
components/accordion/accordion.mjs 20.38 KiB
components/button/button.mjs 4.4 KiB
components/character-count/character-count.mjs 20.57 KiB
components/checkboxes/checkboxes.mjs 5.48 KiB
components/error-summary/error-summary.mjs 5.69 KiB
components/exit-this-page/exit-this-page.mjs 15.66 KiB
components/header/header.mjs 3.42 KiB
components/notification-banner/notification-banner.mjs 4.24 KiB
components/radios/radios.mjs 4.47 KiB
components/skip-link/skip-link.mjs 3.55 KiB
components/tabs/tabs.mjs 9.16 KiB

View stats and visualisations on the review app


Action run for 61b64d4

@colinrotherham colinrotherham changed the title Use Nunjucks default() for โ€œfalseyโ€ values Fix Nunjucks default() for โ€œfalseyโ€ values Oct 11, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4327 October 12, 2023 12:37 Inactive
@@ -47,7 +47,7 @@
{# Allow selecting by text content (the value for an option when no value attribute is specified) #}
{% set effectiveValue = item.value | default(item.text) %}
<option {%- if item.value !== undefined %} value="{{ item.value }}"{% endif %}
{{-" selected" if item.selected | default(effectiveValue == params.value if params.value else false, true) }}
{{-" selected" if item.selected | default((effectiveValue == params.value and item.selected != false) if params.value else false, true) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

When will this trigger? My brain's spinning in circles...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a weird case where govukSelect() can have a selected value but the item itself is unselected ๐Ÿคฏ

So we want to keep the falsey value as long as it's actually false and not null or ""

items:
- value: red
text: Red
- value: green
text: Green
selected: false
- value: blue
text: Blue
value: green

It's covered by this test case:

it('allows item.selected to override value', () => {
const $ = render('select', examples['item selected overrides value'])
const $selectedItem = $('option[value="green"]')
expect($selectedItem.attr('selected')).toBeUndefined()
})

@domoscargin
Copy link
Contributor

Looks good, I just need an explainer on the item.selected stuff because it's still too early for my brain.

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.

I was going to do a foot emoji and a bug emoji for squashing the bug, but let's go with releasing the bug into the garden instead.

๐Ÿ› ๐Ÿž๏ธ

@colinrotherham colinrotherham changed the title Fix Nunjucks default() for โ€œfalseyโ€ values Fix Nunjucks default() values when null, false or "" is provided Oct 19, 2023
@colinrotherham
Copy link
Contributor Author

Thanks @domoscargin just added a CHANGELOG entry too

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4327 October 19, 2023 10:45 Inactive
@colinrotherham colinrotherham merged commit 4b02efc into main Oct 19, 2023
44 checks passed
@colinrotherham colinrotherham deleted the nunjucks-default-falsey branch October 19, 2023 10:54
romaricpascal pushed a commit that referenced this pull request Oct 27, 2023
Fix Nunjucks `default()` values when `null`, `false` or `""` is provided
@romaricpascal romaricpascal mentioned this pull request Dec 8, 2023
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) nunjucks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review Nunjucks default() fallbacks for "falsey" values
3 participants