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

Introduce "add another" component #4420

Merged

Conversation

dnkrj
Copy link
Contributor

@dnkrj dnkrj commented Nov 20, 2024

What

  • Add the component wrapper helper to fieldsets
  • Introduce the add another component based on the MoJ design system and existing component in Whitehall.
  • An empty field and destroy checkboxes for the existing fields are required and displayed to the user when javascript is disabled in keeping with rails conventions.
  • When Javascript is enabled, an "add another" button is introduced to allow users to add copies of the empty field and the checkboxes replaced with "Delete" buttons which hide the fields and checks the appropriate checkbox.

Why

Screenshots

To see further examples of usage, the migration PR for Whitehall can be looked at.

The screenshots are taken with input and checkbox styles added to the page.
This are not present in the code in the PR.

With Javascript

Screenshot 2024-11-25 at 17 45 43 Screenshot 2024-11-25 at 17 45 55

Without Javascript

Screenshot 2024-11-25 at 17 46 56

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4420 November 20, 2024 15:46 Inactive
@dnkrj dnkrj force-pushed the 3139-move-add-another-pattern-to-govukpublishingcomponents branch from b5cc67c to 6d5e632 Compare November 20, 2024 15:47
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4420 November 20, 2024 15:47 Inactive
Copy link
Contributor

@jon-kirwan jon-kirwan left a comment

Choose a reason for hiding this comment

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

It's looking good 👍 I've just added a couple of smallish comments.

Also, I think the add-another styles need to be added to the _all_components.scss stylesheet.

Copy link
Contributor

@jon-kirwan jon-kirwan left a comment

Choose a reason for hiding this comment

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

Are there any accessibility issues with having multiple labels and delete buttons that all share the same name? For example, if a screen reader user navigates the form by buttons using the rotor, could this cause confusion? Would it be worth considering adding a visually hidden element next to each label and delete button to make them uniquely identifiable e.g.

<label>Full name <span class="visually-hidden">1</span></label>

and

<button>Delete <span class="visually-hidden">full name 1</span></button>

@dnkrj
Copy link
Contributor Author

dnkrj commented Nov 25, 2024

Thanks for the review @jon-kirwan!
You're right that the repeated button labels aren't good for accessibility.
It's a little bit difficult to update them programatically since we don't know exactly what the caller will put in so I've updated the code to wrap all of the fields in a numbered fieldset (and updated the screenshots to match).
This follows one of examples for "Providing Accessible Names and Descriptions" in the ARIA Authoring Practices Guide (APG). Let me know what you think!

@dnkrj dnkrj force-pushed the 3139-move-add-another-pattern-to-govukpublishingcomponents branch from 6d5e632 to 6b555fe Compare November 25, 2024 17:42
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4420 November 25, 2024 17:43 Inactive
@dnkrj dnkrj force-pushed the 3139-move-add-another-pattern-to-govukpublishingcomponents branch from 6b555fe to aadf4e5 Compare November 25, 2024 17:49
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4420 November 25, 2024 17:50 Inactive
@dnkrj dnkrj force-pushed the 3139-move-add-another-pattern-to-govukpublishingcomponents branch 2 times, most recently from e2487b7 to f460648 Compare November 26, 2024 11:03
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4420 November 26, 2024 11:04 Inactive
@dnkrj dnkrj force-pushed the 3139-move-add-another-pattern-to-govukpublishingcomponents branch from f460648 to 2b5dd0f Compare November 26, 2024 12:07
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4420 November 26, 2024 12:07 Inactive
@dnkrj dnkrj force-pushed the 3139-move-add-another-pattern-to-govukpublishingcomponents branch from 2b5dd0f to 85cb35d Compare November 26, 2024 12:16
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4420 November 26, 2024 12:17 Inactive
@dnkrj dnkrj force-pushed the 3139-move-add-another-pattern-to-govukpublishingcomponents branch from 85cb35d to 28ed297 Compare November 26, 2024 12:18
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4420 November 26, 2024 12:18 Inactive
@dnkrj dnkrj force-pushed the 3139-move-add-another-pattern-to-govukpublishingcomponents branch from 28ed297 to 263cef8 Compare November 26, 2024 14:42
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4420 November 26, 2024 14:42 Inactive
@jon-kirwan
Copy link
Contributor

Thanks for the review @jon-kirwan! You're right that the repeated button labels aren't good for accessibility. It's a little bit difficult to update them programatically since we don't know exactly what the caller will put in so I've updated the code to wrap all of the fields in a numbered fieldset (and updated the screenshots to match). This follows one of examples for "Providing Accessible Names and Descriptions" in the ARIA Authoring Practices Guide (APG). Let me know what you think!

Thanks, that looks better! There's still an issue with identifying the form components with the rotor. However, when using VoiceOver to navigate the page, the unique fieldset is announced each time you tab to an input, which will be helpful

Copy link
Contributor

@jon-kirwan jon-kirwan 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 and thanks for answering all my questions @dnkrj!

@dnkrj dnkrj force-pushed the 3139-move-add-another-pattern-to-govukpublishingcomponents branch from 263cef8 to 97680d9 Compare November 27, 2024 16:26
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4420 November 27, 2024 16:26 Inactive
@dnkrj dnkrj force-pushed the 3139-move-add-another-pattern-to-govukpublishingcomponents branch from 97680d9 to 5b4fb5c Compare November 27, 2024 16:35
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4420 November 27, 2024 16:35 Inactive
@davidtrussler
Copy link
Contributor

Hello @dnkrj Just been looking at this - looks really good. The only thing that came to mind was to maybe explain a bit more in the documentation about the empty item: I don't think it's immediately obvious what that does and how it should look.

This allows classes to be passed to the fieldset component (needed for the add another component)
This is based on the MoJ component and existing component in Whitehall. It is to be used when users need to add similar information a couple of times, such as several featured links for an organisation.

An empty field and destroy checkboxes for the existing fields are required and displayed to the user when javascript is disabled in keeping with rails conventions.

When Javascript is enabled, an "add another" button is introduced to allow users to add copies of the empty field and the checkboxes replaced with "Delete" buttons which hide the fields and checks the appropriate checkbox.
@dnkrj dnkrj force-pushed the 3139-move-add-another-pattern-to-govukpublishingcomponents branch from 5b4fb5c to 0602dc3 Compare November 28, 2024 13:10
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4420 November 28, 2024 13:10 Inactive
@dnkrj dnkrj merged commit add8aff into main Nov 28, 2024
12 checks passed
@dnkrj dnkrj deleted the 3139-move-add-another-pattern-to-govukpublishingcomponents branch November 28, 2024 13:12
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.

5 participants