Closed malcolmhire closed 5 years ago
Hi @malcolmhire , thanks for raising this. As you have correctly identified, there is no built-in way to merge objects in Nunjucks without creating a filter. Relying on our users to create a filter, as we can't ship one with a component, isn't something we'd want to do.
One possible solution would be to move the pattern
attribute from the object and add it directly to the component, but that might not be as flexible if users want to create a custom pattern on data-inputs. I'm not sure how often that would happen though.
I have also created a ticket in our backlog so we can track this.
I wonder if objects could be merged using a for loop
Hi @malcolmhire , thanks for raising this. As you have correctly identified, there is no built-in way to merge objects in Nunjucks without creating a filter. Relying on our users to create a filter, as we can't ship one with a component, isn't something we'd want to do.
One possible solution would be to move the
pattern
attribute from the object and add it directly to the component, but that might not be as flexible if users wan't to create a custom pattern on data-input. I'm not sure how often that would happen though.I have also created a ticket in our backlog so we can track this.
When you say 'directly to the component' do you mean adding it directly to the govukInput? I thought about adding it like that with the pattern as part of the govukInput, this would then leave open attributes object, but this wouldn't be consistent with all the other components so ditched that idea.
@igloosi is this the sort of thing you were meaning?
{{ govukInput({ label: { text: "foo", }, id: "foo", name: "foo" value: "bar", type: "number", pattern: "[0-9]*" })
@malcolmhire something along those lines yes, but you're right, it lacks consistency
~wondering if we can get user attributes object and component's internal object, push both to an array an then internally in the component loop over the array?~
perhaps we might have to allow an array of objects to be passed as attributes
@malcolmhire there is a check in Nunjucks one can do check if array is empty
{% if array | length %}
so attributes is passed as an array of objects we could loop over them and merge them with the default attributes otherwise juts loop over them
Nunjucks array loop
{% for item in items %}
Nunjucks object loop
{% for key, value in item %}
your thoughts?
@malcolmhire something along those lines yes, but you're right, it lacks consistency
wondering if we can get user attributes object and component's internal object, push both to an array an then internally in the component loop over the array?
perhaps we might have to allow an array of objects to be passed as attributes
@malcolmhire there is a check in Nunjucks one can do check if array is empty
{% if array | length %}
so attributes is passed as an array of objects we could loop over them and merge them with the default attributes otherwise juts loop over them
Nunjucks array loop
{% for item in items %}
Nunjucks object loop
{% for key, value in item %}
your thoughts?
Hey, sorry for the late reply, been on leave.
I think that idea will work but again is it not consistent with the other components as attributes are a single object on all other components (unless I've missed something). Thoughts?
@malcolmhire yeah they are. I also did some quick testing and there isn't a reliable way to check with something | length
as it will return 1 for an object as well.
I've spoken with Hanna about this, and we think these are the ways forward:
{{ govukInput({ __additionalAttributes: {} }) }}
We'd need to ensure that the additional attributes overrides existing attributes before they're applied.
I've gone over this with @36degrees and had a separate discussion with @hannalaakso.
We think there are risks with option 1. which I've updated in the previous comment, and that we should go with an alternative that avoids this.
So we are proposing to add pattern
as a top level option for the text input component macro. (option 4.)
Which will allow the user to set attributes without having to remove the defaults.
The reasoning for this is we think that the attributes
option should not be be use internally if possible. If we find attributes that are useful internally, we should consider if they're in the HTML spec to add them as a top level API for the macro.
We would then do a technical spike into option 2. as we think that being able to use global functions has enough evidence of usefulness but we don't want to block this until we've figured that out.
@colinrotherham @malcolmhire how does that sound?
Yeah sounds good to me.
Just like the work to enable autocomplete
in https://github.com/alphagov/govuk-frontend/pull/1146 but for pattern
as shown below?
Also if absolutely required we can still add attributes
too?
{{ govukDateInput({
items: [
{
// If omitted, default is still set
pattern: "[0-9]*",
// Additional attributes
attributes: {
min: "1",
max: "31"
}
}
]
}) }}
@colinrotherham yes, we'll still keep the attributes for users of the macro to add their additional attributes 👍
Perfect. This is now supported by https://github.com/alphagov/govuk-frontend/pull/1172
This looks good to me 😀
Can this be closed now or shall we keep it open for the further Nunjucks object merging changes?
Currently, there is no way of adding extra attributes to each of the date input fields, this would be a fairly easy update but the macro already sets attributes on Line 67 and as there no built in way to merge objects in Nunjucks (well not that I know of) this would require a filter, but this would need to added application side when initializing Nunjucks.
Any ideas how to achieve this?