-
Notifications
You must be signed in to change notification settings - Fork 328
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 input value not being set if the value was '0' #4963
Conversation
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for 913b771 |
This comment was marked as outdated.
This comment was marked as outdated.
1b28d72
to
4c4ab34
Compare
4c4ab34
to
161c516
Compare
161c516
to
401d3d8
Compare
401d3d8
to
cb00f77
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for picking this up and congrats on your first contribution to GOV.UK Frontend on the team 😎 🎉
There's a couple of things that aren't quite right at the minute – in particular it looks like the changes in #4770 might have been out of date with some other changes to add the autocapitalize
attribute, sorry we'd missed that!
Let me know if it'd be helpful to chat through any of this or if you've got any questions!
optional: true | ||
}, | ||
autocomplete: { | ||
value: params.autocomplete | default("none", true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this has changed from #4770 and changes the behaviour slightly.
Previously, if params.autocomplete
wasn't set, we wouldn't output an autocomplete
attribute.
Now, if params.autocomplete
isn't set, we output a default autocomplete="none"
which isn't valid.
This is one of the reasons the tests are failing – we run an html validator against all of the examples and it's erroring with '"none" is not a valid autocomplete token or field name'.
{%- if params.autocomplete %} autocomplete="{{ params.autocomplete }}"{% endif %} | ||
{%- if params.pattern %} pattern="{{ params.pattern }}"{% endif %} | ||
{%- if params.inputmode %} inputmode="{{ params.inputmode }}"{% endif %} | ||
{%- if params.autocapitalize %} autocapitalize="{{ params.autocapitalize }}"{% endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -455,22 +446,4 @@ describe('Input', () => { | |||
expect($prefixBeforeSuffix.length).toBeTruthy() | |||
}) | |||
}) | |||
|
|||
describe('when it includes the input wrapper', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue Why have these tests been removed?
name: with-zero-value | ||
label: | ||
text: With zero value | ||
value: !!int 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question Is the casting definitely necessary here? My understanding was that YAML should treat this is an integer already.
@@ -71,6 +71,13 @@ describe('Input', () => { | |||
expect($component.val()).toBe('QQ 12 34 56 C') | |||
}) | |||
|
|||
it('renders with zero value', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise Thanks for adding a test for this 🙌🏻
cb00f77
to
4c2844a
Compare
4c2844a
to
5846560
Compare
5846560
to
5383522
Compare
5383522
to
06bd485
Compare
06bd485
to
013fa29
Compare
013fa29
to
51d1eb4
Compare
51d1eb4
to
dfe5380
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great, thanks for addressing those comments.
One last thing – we'll want to add a changelog entry so that users know we've fixed this.
For a change like this we'd usually list it under 'Fixes', and just use the PR title, crediting anyone involved or reporting or fixing it as appropriate.
It might be worth making the PR title slightly more descriptive so we end up with something like this:
### Fixes
We've made fixes to GOV.UK Frontend in the following pull requests:
<!-- Existing fixes already in the changelog -->
- [#4963: Fix input value not being set if the value was '0'](https://github.com/alphagov/govuk-frontend/pull/4963) – thanks to @ dwp-dmitri-algazin for reporting this issue
dfe5380
to
01df7cd
Compare
There was a problem hiding this 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! 🙌🏻
01df7cd
to
bfb1a8f
Compare
bfb1a8f
to
686a087
Compare
Fixes #4669. Input component now uses govukAttributes macro to validate and format attributes. Test and example added for edge case outlined in issue #4669. Co-authored-by: Colin Rotherham <[email protected]>
686a087
to
913b771
Compare
What
Input component now uses the govukAttributes macro to validate and format attributes. Test and example added for edge-case outlined in issue #4669.
Why
Issue #4669 outlines a case where setting 'value' of the Input component to 0 (integer) would not set the value of the input component. This is because the template logic would see '0' as false-y and so not set the value attribute. The govukAttributes takes this case into account and so using it means that this no longer occurs.
This is based from #4770. It adds tests and the input wrapper classes and attributes back.