alphagov / govuk-frontend

GOV.UK Frontend contains the code you need to start building a user interface for government platforms and services.
https://frontend.design-system.service.gov.uk/
MIT License
1.18k stars 324 forks source link

Ability to control the id and name attributes of the Date input items #903

Closed davidjamesstone closed 6 years ago

davidjamesstone commented 6 years ago

Currently the id and name are auto generated in the macro:

    "id": params.id + "-" + item.name,
    "name": params.name + "-" + item.name

I need more control over these. For example, let say I am capturing a DoB of a person:

<input type="number" name="person[dob].day" />
<input type="number" name="person[dob].month" />
<input type="number" name="person[dob].year" />

Would it be possible to include this capability?

kr8n3r commented 6 years ago

Hi @davidjamesstone, you can provide id and name to each item: https://github.com/alphagov/govuk-frontend/blob/master/src/components/date-input/template.njk#L66

Are you using the latest released version of Frontend?

davidjamesstone commented 6 years ago

Fantastic. I was using v1.0.0.

Many thanks

davidjamesstone commented 6 years ago

I think there's a bug with this as it stands.

While I am now able to control the id/name of each item, the logic that sets the input class width modifier is flawed as it's based on the hardcoded string 'year', not the passed name:

https://github.com/alphagov/govuk-frontend/blob/8f2fe567865729716fa4a04b0c6eeb5860da7441/src/components/date-input/template.njk#L58

Also, I would rather the logic for the item.name was like this: name: item.name if item.name else (params.name + "-" + item.name) rather than: name: (params.name + "-" + item.name) if params.name else item.name,

This makes it work like the id attribute. Basically, if I pass you a name you should use it regardless.

36degrees commented 6 years ago

While I am now able to control the id/name of each item, the logic that sets the input class width modifier is flawed as it's based on the hardcoded string 'year', not the passed name:

This is a temporary measure to preserve current behaviour which we intend to remove in the next breaking release. We expect that going forward users will pass in explicit width classes as are found in the default items.

Also, I would rather the logic for the item.name was like this: name: item.name if item.name else (params.name + "-" + item.name) rather than: name: (params.name + "-" + item.name) if params.name else item.name,

This makes it work like the id attribute. Basically, if I pass you a name you should use it regardless.

I'm not sure I understand your proposal – if we expand it to make what's going on clearer:

if item.name 
  item.name
else
  params.name + "-" + item.name

In the else block we are trying to append 'item.name' despite knowing it to be falsey, so every input would have the same name – whatever params.name was, followed by an underscore?

The current logic means that params.name (the top level name parameter) acts as a prefix and only if it's provided – if you omit if, then the name on each input will be exactly what you pass as item.name. It could definitely be clearer though – perhaps we should rename it to namePrefix?

davidjamesstone commented 6 years ago

Sorry - I rushed the code in the proposal there.

I didn't mean use item.name as a suffix even if it's known to be falsey. I meant if I pass you an item[].name, I expected the macro to use it as-is (i.e. no prefix), regardless of whether I passed a name on the top level. Doing this will then make it work like it's actually docum ented:

Optional item-specific name attribute. If provided, it will be used instead of the generated name

Renaming the top level name to namePrefix improves things - it's more explicit but I'm suggesting the prefix/generated name is only used if I don't supply one explicitly in the items[].name.

On the first point, I'm still confused, sorry. At the moment I can't pass both a name for each individual item using item[].name AND correctly control the year inputs' width. Am I doing something wrong or is this just semantics of what you're calling a "temporary measure", I'm calling a "bug"?!?

Also, two minor points:

  1. It isn't documented that the DateInput items can also receive a label in https://github.com/alphagov/govuk-frontend/blob/master/package/components/date-input/README.md#component-arguments.

items[].label

  1. The documentation for item[].name isn't correct:
items.{}.name array Yes

should be

items.{}.name string No

Thanks for your time.

36degrees commented 6 years ago

Sorry - I rushed the code in the proposal there.

I didn't mean use item.name as a suffix even if it's known to be falsey. I meant if I pass you an item[].name, I expected the macro to use it as-is (i.e. no prefix), regardless of whether I passed a name on the top level. Doing this will then make it work like it's actually docum ented:

Optional item-specific name attribute. If provided, it will be used instead of the generated name

Renaming the top level name to namePrefix improves things - it's more explicit but I'm suggesting the prefix/generated name is only used if I don't supply one explicitly in the items[].name.

Thanks for clarifying. The documentation here is unfortunately wrong - item.name is a 'required' field, so should always be provided – if you don't provide an item.name, there is no generated name to fall back to, so all three fields will just end up with the same name. The line in the documentation that you refer to was actually updated in #895 to remove the incorrect 'optional' clause, but this change has yet to be released.

With all of that in mind, I think it would make sense just to rename params.name to params.namePrefix to try and make it clearer what that argument actually does. Thoughts?

On the first point, I'm still confused, sorry. At the moment I can't pass both a name for each individual item using item[].name AND correctly control the year inputs' width. Am I doing something wrong or is this just semantics of what you're calling a "temporary measure", I'm calling a "bug"?!?

I think I've been able to recreate this now – inputWidthClass should just be preserving existing behaviour in the absence of any explicit input width class, but it seems to be overriding any other width class that you pass in via item.classes?

The intention with that line was to preserve the existing behaviour to avoid having to make a breaking release, but any modifier passed via item.classes should always override it. I'll try and work out a fix for that.

Also, two minor points:

It isn't documented that the DateInput items can also receive a label in https://github.com/alphagov/govuk-frontend/blob/master/package/components/date-input/README.md#component-arguments. items[].label

The documentation for item[].name isn't correct: items.{}.name array Yes should be

items.{}.name string No Thanks for your time.

Great spots – thank you for helping us to improve the docs 👍 Would you be willing to open a PR to update those? You'd need to edit README.njk and then run gulp generate:readme.

davidjamesstone commented 6 years ago

Of course, item.name must be required! Realising that, I think namePrefix is the best solution. The documentation for item.name could then read something like:

Required item-specific name attribute. Will be prefixed with `params.namePrefix` if supplied.

Alternatively, consider dropping the prefixing/autogenerating altogether?

I'm not sure what the value is now. If I want a prefix I can easily add one to item[].name myself. Dropping it makes the docs clearer and it'll prevent any future issues being raised along the lines of "Can I control the prefix separator character, I don't like -, I want _".


Hopefully this clears up the "width" issue. See the image below, the first example is fine, the next two don't have the correct width for the year input field. This is due to the macro hardcoding the `if item.name === 'year')

{# Width of the year input is fine if I set the item[].name to 'year' #}
  {{ govukDateInput({
    id: "dob1",
    fieldset: {
      legend: {
        text: "1. What is your date of birth?",
        isPageHeading: true,
        classes: "govuk-fieldset__legend--xl"
      }
    },
    items: [
      { name: "day" },
      { name: "month" },
      { name: "year" }
    ]
  }) }}

  {# Set to anything else breaks the width of the input #}
  {{ govukDateInput({
    id: "dob2",
    fieldset: {
      legend: {
        text: "2. What is your date of birth?",
        isPageHeading: true,
        classes: "govuk-fieldset__legend--xl"
      }
    },
    items: [
      { label: 'Day', name: "dob[day]" },
      { label: 'Month', name: "dob[month]" },
      { label: 'Year', name: "dob[year]" }
    ]
  }) }}

  {# Trying to explicitly set it using `classes` has no effect. I end up with both `govuk-input--width-2` and `govuk-input--width-4` on the year input #}
  {{ govukDateInput({
    id: "dob3",
    fieldset: {
      legend: {
        text: "2. What is your date of birth?",
        isPageHeading: true,
        classes: "govuk-fieldset__legend--xl"
      }
    },
    items: [
      { label: 'Day', name: "dob[day]" },
      { label: 'Month', name: "dob[month]" },
      { label: 'Year', name: "dob[year]", classes: 'govuk-input--width-4' }
    ]
  }) }}
screen shot 2018-07-16 at 15 55 03

--

Happy to provide a PR on the documetation issues....

Thanks @36degrees

36degrees commented 6 years ago

Thanks, I will keep an eye out for a documentation PR 👍 🎉

I'm going to split this into two issues so that we can track each one separately:

I've tried my best to summarise each issue, but please do feel free to add anything I might have missed.

I think that covers everything discussed in this issue, so I'm going to close this now. Please do re-open it or file another issue if you need to.

36degrees commented 6 years ago

@davidjamesstone just to let you know that we've now merged #984 which addresses #909 by renaming the name argument to namePrefix – it'll be part of the next release of Frontend, which we expect to be v2.0.

Thanks again for raising this and helping to improve Frontend! 👍