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 another: fix problem in createRemoveButtons method: #4586

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

davidtrussler
Copy link
Contributor

What

This change updates a querySelectorAll call to be made on the module rather than the document.

Why

As it is this creates multiple "Delete" buttons if there are more than one instance of the component on the page (i.e. one per instance).

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4586 January 23, 2025 12:16 Inactive
@davidtrussler davidtrussler force-pushed the Fix-createRemoveButtons-method branch from a6fdfec to 5bea69c Compare January 23, 2025 12:23
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4586 January 23, 2025 12:24 Inactive
@davidtrussler davidtrussler marked this pull request as ready for review January 23, 2025 12:26
Copy link
Contributor

@AshGDS AshGDS left a comment

Choose a reason for hiding this comment

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

LGTM 👍 The component example throws an unrelated JavaScript error, does that need to be investigated?

@davidtrussler davidtrussler force-pushed the Fix-createRemoveButtons-method branch from 5bea69c to e7a0f2c Compare January 27, 2025 10:53
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4586 January 27, 2025 10:53 Inactive
- This creates multiple "Delete" buttons
@davidtrussler davidtrussler force-pushed the Fix-createRemoveButtons-method branch from e7a0f2c to 792ce10 Compare January 27, 2025 10:55
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4586 January 27, 2025 10:55 Inactive
@davidtrussler
Copy link
Contributor Author

LGTM 👍 The component example throws an unrelated JavaScript error, does that need to be investigated?

Hello @AshGDS Thanks for the review and yes, good spot, there is an error that should probably be looked at. I'll raise an issue separately.

@davidtrussler davidtrussler merged commit 6d7ee65 into main Jan 27, 2025
12 checks passed
@davidtrussler davidtrussler deleted the Fix-createRemoveButtons-method branch January 27, 2025 11:40
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