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

Add the password input component #4176

Merged
merged 3 commits into from
Sep 4, 2024
Merged

Add the password input component #4176

merged 3 commits into from
Sep 4, 2024

Conversation

AshGDS
Copy link
Contributor

@AshGDS AshGDS commented Aug 27, 2024

What / Why

  • Adds the password input component from the GOVUK Design System - see their docs here
  • There is a bug which I wasn't sure what to do about. Our component docs say that text in components should be translatable - so I tried to allow the Show/Hide button text to be overwritten. However, the button text gets changed by the GOVUK Frontend JavaScript on click. Therefore even if we change the text in the HTML, it gets overwritten as soon as you click the button. Therefore I've added the ability to change the button text in the template file, but I haven't made it known in the yml docs that it's a feature.
  • Hope everything's up to standard - first time adding a component!
  • Trello card: https://trello.com/c/Gj5yIsxH/298-implement-password-input-component

Visual Changes

  • The addition of the component has added visual changes to review.

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 August 27, 2024 15:10 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 August 27, 2024 15:23 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 August 27, 2024 15:28 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 August 28, 2024 13:28 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 August 29, 2024 08:56 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 August 29, 2024 13:50 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 August 29, 2024 14:06 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 August 29, 2024 15:01 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 August 29, 2024 15:13 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 August 30, 2024 13:37 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 August 30, 2024 13:44 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 August 30, 2024 14:04 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 August 30, 2024 14:08 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 August 30, 2024 14:10 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 August 30, 2024 14:13 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 August 30, 2024 14:25 Inactive
@AshGDS AshGDS self-assigned this Aug 30, 2024
@AshGDS AshGDS marked this pull request as ready for review August 30, 2024 14:47
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 August 30, 2024 14:48 Inactive
Copy link
Contributor

@andysellick andysellick 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! Just a few minor things to look at.


label_text ||= t("components.password_input.label")

# This overrides the 'Show/Hide' button text, but this button text is getting overwritten by GOVUK Frontend JS currently when you interact with the button. Therefore this functionality is not being exposed in the docs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth asking the Design System team if it's possible/planned to be able to override the text values in https://github.com/alphagov/govuk-frontend/blob/main/packages/govuk-frontend/src/govuk/components/password-input/password-input.mjs#L222-L231

Copy link
Contributor Author

Choose a reason for hiding this comment

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

config/locales/en.yml Outdated Show resolved Hide resolved
config/locales/en.yml Show resolved Hide resolved
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 September 3, 2024 14:46 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 September 3, 2024 14:53 Inactive
@AshGDS
Copy link
Contributor Author

AshGDS commented Sep 3, 2024

Thanks for reviewing @andysellick 👍 I've made every change requested, so this should be ready for another review.

it "renders a password input component correctly by default" do
render_component({})
puts rendered
Copy link
Contributor

Choose a reason for hiding this comment

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

Debug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, thanks!

Copy link
Contributor

@andysellick andysellick left a comment

Choose a reason for hiding this comment

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

Nice work. One bit of debug to remove, and obviously squash the commits, but happy to approve ahead of that 👍

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4176 September 4, 2024 10:04 Inactive
@AshGDS AshGDS changed the title Password input component Add the password input component Sep 4, 2024
@AshGDS AshGDS merged commit 5a7821b into main Sep 4, 2024
12 checks passed
@AshGDS AshGDS deleted the password-input-component branch September 4, 2024 10:08
@JamesCGDS JamesCGDS mentioned this pull request Sep 10, 2024
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.

3 participants