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

Rename name of date-input component to namePrefix #984

Merged
merged 3 commits into from Sep 6, 2018

Conversation

hannalaakso
Copy link
Member

The name argument on the date-input component acts as a prefix for each input's name, but this behaviour is not clear or consistent with how we use name in other components.

This PR renames it to namePrefix. This is consistent with other components (like checkboxes and radios) which use the name idPrefix to explain similar functionality.

Fixes #909

https://trello.com/c/CFN5bvev/1392-2-behaviour-of-date-inputs-name-argument-is-not-clear

This is better reflective of the purpose of the argument, which is to prefix the `name`
attribute of `items`. This naming is consistent with other components which use the
name `idPrefix` to explain similar functionality.
@hannalaakso hannalaakso added this to the v2.0.0 milestone Sep 6, 2018
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-984 September 6, 2018 15:08 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-984 September 6, 2018 15:10 Inactive
Copy link
Member

@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.

LGTM 👍

@@ -72,6 +72,14 @@

([PR #969](https://github.com/alphagov/govuk-frontend/pull/969))

- Rename `name` argument of date-input component to `namePrefix`.

This is better reflective of the purpose of the argument, which is to prefix the `name` attribute of `items`. This is consistent with other components which use the name `idPrefix` to explain similar functionality.
Copy link
Member

Choose a reason for hiding this comment

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

Great changelog entry – I think this gets the intention across well, and makes it clear what the user needs to do 👍

@@ -55,7 +55,7 @@
text: 'No'
},
{
text: 'Optional name. This is used to compose the name attribute for each item.'
text: 'Optional prefix. This is used to prefix each `item.name`.'
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's worth mentioning that they'll be joined using "-"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done 👍 I thought initially about adding it but it felt like too much about the implementation. But it's probably useful information.

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.

Great work

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-984 September 6, 2018 15:46 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Behaviour of date input's name argument is not clear
4 participants