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

Normalise falsey values to nil to prevent lang="false" #1021

Merged
merged 3 commits into from
Aug 6, 2019

Conversation

injms
Copy link
Contributor

@injms injms commented Aug 5, 2019

What

Checks that the locale attributes being passed into the subscription links components are falsey - if falsey then nil is used.

Why

When a false was being used as a parameter in the subscription components, it was being cast and used as a string - leading to lang="false". Not a big deal, since (according to the spec) this should be ignored, but worth fixing.

Visual Changes

None.

View Changes

https://govuk-publishing-compo-pr-1021.herokuapp.com/

@bevanloon bevanloon temporarily deployed to govuk-publishing-compo-pr-1021 August 5, 2019 16:31 Inactive
@injms injms requested a review from vanitabarrett August 5, 2019 16:43
@injms injms force-pushed the subscription-links-translation-patch branch from b625347 to cd26bfd Compare August 5, 2019 16:46
@bevanloon bevanloon temporarily deployed to govuk-publishing-compo-pr-1021 August 5, 2019 16:46 Inactive
Copy link
Contributor

@tijmenb tijmenb left a comment

Choose a reason for hiding this comment

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

👍 I wonder if this needs a test to prevent regressions.

Copy link
Contributor

@tijmenb tijmenb left a comment

Choose a reason for hiding this comment

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

💯

@injms injms merged commit 5576d1f into master Aug 6, 2019
@injms injms deleted the subscription-links-translation-patch branch August 6, 2019 12:27
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.

3 participants