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

Enable pattern attribute for input (+ custom date input attributes) #1172

Merged
merged 3 commits into from
Mar 5, 2019

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Feb 4, 2019

We're converting a service from GOV.UK Elements to GOV.UK Frontend and noticed we can't provide custom attributes to our day/month/year fields.

Here's a pull request to add the feature.

As there's no "object merge" nunjucks filter the pattern: '[0-9]*' default attribute must be added manually when the entire attributes object is overridden from the defaults.

@36degrees
Copy link
Contributor

This would fix #995, if we're happy with this as a solution.

I am a little concerned about users forgetting (or not realising) that they need to pass in the pattern attribute – would need to be careful about how we documented this.

@colinrotherham
Copy link
Contributor Author

@36degrees Yeah I thought the same.

I added a new global called merge which combined the custom attribute over the defaults, but then realised most people's nunjucks setups would need to re-add the global.

Would have been like:

{
  autocomplete: item.autocomplete,
  attributes: merge(item.attributes, { pattern: "[0-9]*" })
}

Sadly nunjucks doesn't provide object merging.

Copy link

@HughePaul HughePaul left a comment

Choose a reason for hiding this comment

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

We want to add maxlength to the fields, as this worked much better in testing. However this requires the type to not be number as you can't set a maxlength for number fields (although you can set a min and max with some success)

@malcolmhire
Copy link

This is the same way I have done it in my forked version of govuk-frontend, will be interesting if this gets accepted as I was also concerned that users might forget to add the pattern attribute.

@colinrotherham
Copy link
Contributor Author

Rebased with latest master

@NickColley NickColley added the awaiting triage Needs triaging by team label Feb 26, 2019
@NickColley
Copy link
Contributor

Hanna and I have reviewed this, we like this approach but want to consider the others too.

If we were to go with this approach we'd need to update the YAML description to be clear that setting this attributes requires you to add pattern back in.

You can see #995 (comment) for more details on what we've talked about.

@colinrotherham colinrotherham force-pushed the date-input-attributes branch 2 times, most recently from 3df9ebb to 35459df Compare March 4, 2019 15:03
@colinrotherham
Copy link
Contributor Author

@NickColley @hannalaakso If you'd like to go with this approach, I've updated both the yaml description and changelog entry to include pattern: '[0-9]*'.

Looking forward to seeing __additionalAttributes and/or a merge global filter in future 👍

NickColley
NickColley previously approved these changes Mar 4, 2019
Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

I think this is a good compromise that will also unblock @malcolmhire and we can still consider a more robust developer friendly way in the future as discussed.

The work itself is clearly documented at the point of use, and implemented well with tests, thanks a lot for this Colin.

Will require a second review.

@NickColley NickColley added this to the 2.8.0 milestone Mar 4, 2019
@colinrotherham colinrotherham force-pushed the date-input-attributes branch from 35459df to e4485b5 Compare March 5, 2019 09:46
@NickColley NickColley dismissed their stale review March 5, 2019 10:38

Discussion with team

@NickColley
Copy link
Contributor

I've talked this through with the team and we think there's an alternative to this that avoids some of the negative aspects of this approach without making breaking changes:
#995 (comment)

@colinrotherham colinrotherham changed the title Date input attributes Enable pattern attribute for input (+ custom date input attributes) Mar 5, 2019
@@ -162,3 +166,11 @@ examples:
id: input-with-autocomplete-attribute
name: postcode
autocomplete: postal-code
- name: with pattern attribute

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

I've reviewed the live examples and they are outputting correctly.

And the api changes to the macros look great.

There's a lot going on here though so would appreciate another proper good look from someone on the team.

Copy link
Contributor

@36degrees 36degrees 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, thanks @colinrotherham 👍

I've added one minor comment about one of the tests, but happy to merge as-is if you prefer.

src/components/date-input/template.test.js Outdated Show resolved Hide resolved
@colinrotherham colinrotherham force-pushed the date-input-attributes branch from 178afb8 to b863be5 Compare March 5, 2019 13:52
@colinrotherham
Copy link
Contributor Author

Thanks @36degrees.

I've removed that extra assertion, it was definitely already covered.

All ready to merge if you're happy 😊

@NickColley
Copy link
Contributor

Going to run out of superlatives to describe these pull requests thanks so much for your help Colin.

👏

@NickColley NickColley merged commit 415baac into alphagov:master Mar 5, 2019
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.

6 participants