-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add password input #481
Add password input #481
Conversation
✅ Deploy Preview for govuk-form-builder ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
9ba614a
to
c7a92df
Compare
aa8bd49
to
b8b5df8
Compare
@@ -1,7 +1,7 @@ | |||
describe GOVUKDesignSystemFormBuilder::FormBuilder do | |||
include_context 'setup builder' | |||
|
|||
describe '#date_input_group' do | |||
describe '#govuk_date_field' do |
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.
Spotted a historic error, unrelated to PR 🙈
d32f161
to
d316ce7
Compare
@@ -0,0 +1,31 @@ | |||
--- | |||
title: Password fields |
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.
To be consistent with both the Design System and the documentation for the other form builder fields.
May need to update other occurrences of ‘field’ to use ‘input’, too.
title: Password fields | |
title: Password input |
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.
I thought about that but I think consistency with Rails is more important here and Rails names these password_field.
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.
Could argue the case either way, but the existing form builder documentation already uses ‘input’ for the page titles (‘Date input’, ‘Text input’). I think that’s the right way to go; use the name of the component in the Design System, then in the documentation, talk about fields when relating to implementation.
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.
Addressed in 1f65c86
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.
Just did a very quick pass. Main points:
- Consistently use ‘Password input’ for the name of this field
- Don’t use examples that go against the guidance for this input
guide/lib/examples/password.rb
Outdated
module Password | ||
def default_password | ||
<<~PASSWORD | ||
= f.govuk_password_field :password, label: { text: "Enter your password" } |
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.
To be consistent with the example used in the Design System documentation.
= f.govuk_password_field :password, label: { text: "Enter your password" } | |
= f.govuk_password_field :password, label: { text: "Password" } |
guide/lib/examples/password.rb
Outdated
label: { text: "Enter your PIN", tag: "h2", size: "l" }, | ||
caption: { text: "Security", size: "m" }, | ||
hint: { text: "Your PIN was emailed to you when you registered for this service" }) |
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.
Should probably use another example, only referring to passwords, as the Design System guidance states that this input should only be used for passwords, and not other types of security information.
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.
Fair - I need unique attribute names here which is why I used PIN. I'll rename to password_1
, password_2
and password_3
.
guide/lib/examples/password.rb
Outdated
= f.govuk_password_field(:secret_code, | ||
label: { text: "Enter your secret code" }, | ||
show_password_text: "Display", | ||
hide_password_text: "Conceal", | ||
show_password_aria_label_text: "Display the secret code", | ||
hide_password_aria_label_text: "Conceal the secret code", | ||
password_shown_announcement_text: "The secret code has been displayed", | ||
password_hidden_announcement_text: "The secret code has been concealed") |
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.
Same as above. Maybe these examples could demonstrate using these parameters to enable localisation?
guide/lib/helpers/link_helpers.rb
Outdated
"File upload" => "/form-elements/file-upload/", | ||
"Password fields" => "/form-elements/password/", | ||
"Radios" => "/form-elements/radios/", |
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.
As previous comment, and also to be internally consistent with URL slugs:
"File upload" => "/form-elements/file-upload/", | |
"Password fields" => "/form-elements/password/", | |
"Radios" => "/form-elements/radios/", | |
"File upload" => "/form-elements/file-upload/", | |
"Password input" => "/form-elements/password-input/", | |
"Radios" => "/form-elements/radios/", |
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.
ok - I'll switch it in the guide but keep the Rails-style method name.
6cd09b6
to
507a0d1
Compare
3bed57a
to
1f65c86
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.
Code looks good to me. 👍
Any thoughts on whether this counts as a breaking change or not? On the one hand it’s a new feature, but on the other it re-uses the existing govuk_password_field
method.
Whilst I think in most circumstances, having the show/hide button appear on existing password fields should be fine (and good) - I guess the changed markup might break some tests, and possibly there might be some layouts which need adjusting to make room for the extra button? 🤔
Yeah I was going to cover that in the release notes. Technically breaking but the old one didn't exactly do very much to start with, can't really see it causing any problems. |
Originally I thought we'd be able to build on this but it appears the new password input[0] doesn't really have too much in common with the other text inputs. For example, it doesn't allow the width to be configured [0] alphagov/govuk-design-system#3355
* the i18n data attributes should really be used for localisation, they're now Italian (thanks @edujackedu!) * the official component is called 'password input' rather than 'password field'. I've changed this around in the guide but left the actual code as #govuk_password_field so it's consistent with Rails' other form helpers * renamed the example attributes from PIN and secret code to password_1, password_2, password_3. They need to be unique and not encourage people to use this component for things other than passwords
1f65c86
to
c1695b0
Compare
Points have been addressed and they're in the guide which we can easily tweak post release.
The 'password input' is a new form component added to version 5.3.0 of the GOV.UK Design System.
This change attempts to implement it with all of its custom behaviour and document it in the guide.