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 component wrapper helper to textarea #4574

Merged
merged 3 commits into from
Jan 22, 2025
Merged

Add component wrapper helper to textarea #4574

merged 3 commits into from
Jan 22, 2025

Conversation

andysellick
Copy link
Contributor

@andysellick andysellick commented Jan 20, 2025

What

Apply the component wrapper helper to the textarea component.

  • breaking change, renames id option to textarea_id as this is applied to a child element and id is applied to the parent by the component wrapper
  • removes the shared helper, instead relying on the component wrapper helper to provide the same functionality for margin_bottom
  • removes a bit of code that sets the margin back to normal if an invalid value is passed, but this outcome is now handled by the component wrapper

To deal with this breaking change, other PRs will need to be raised in applications as follows ahead of deployment.

This change also impacts the character count component, which wraps the textarea component. The character count component accepts an id option, which is then passed to the textarea to be the id of the textarea element (not the component id). I've updated the component to explicitly pass this when it exists, because there was a corner case where passing an id as part of the textarea object overwrote the value of id passed directly to character count. Hopefully the tests clarify this behaviour (without causing a breaking change to the character count component).

Why

The code for margin_bottom is moving from the shared helper to the component wrapper helper, so we need to remove any instance of this before the work can be completed. Additionally, having the component wrapper on a component helps reduce code and maintenance effort.

Visual Changes

None.

Trello card: https://trello.com/c/GQ1p2oSC/438-not-doing-add-component-wrapper-helper-to-form-textarea-component

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.

Looks ok to me, but the Percy diff implies something is up. Can you confirm if the Percy diff is what you expect?

@@ -102,7 +102,7 @@ examples:
<%= component %>
<% end %>
data:
id: "contextual-guidance-id"
textarea_id: "contextual-guidance-id"
label:
text: "Title"
bold: true
Copy link
Contributor

Choose a reason for hiding this comment

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

This file needs the uses_component_wrapper_helper: true line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, have added. Looking into the visual diff now 👍

@andysellick
Copy link
Contributor Author

@AshGDS finally fixed the visual diff - there was a surprisingly complex thing happening with the textarea and the elements that followed it, but I think I've finally got it 👍

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 👍

- breaking change, renames `id` option to `textarea_id` as this is applied to a child element and `id` is applied to the parent by the component wrapper
- removes the shared helper, instead relying on the component wrapper helper to provide the same functionality
- removes a bit of code that sets the margin back to normal if an invalid value is passed, but this outcome is now handled by the component wrapper
- also improve tests, remove redundant data attributes test (component doesn't accept data attributes)
@andysellick andysellick changed the title [DO NOT MERGE] Add component wrapper helper to textarea Add component wrapper helper to textarea Jan 22, 2025
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4574 January 22, 2025 10:09 Inactive
@andysellick andysellick merged commit e657557 into main Jan 22, 2025
12 checks passed
@andysellick andysellick deleted the textarea-cw branch January 22, 2025 10:12
unoduetre added a commit that referenced this pull request Jan 22, 2025
* Remove margin top from search component ([PR #4581](#4581))
* **BREAKING** Add component wrapper helper to textarea ([PR #4574](#4574))
* Remove margin top from print link ([PR #4577](#4577))
* **BREAKING** Use component wrapper on print link component  ([PR #4576](#4576))
* Refactor single page notification component ([PR #4501](#4501))
* Remove shared helper from button component ([PR #4569](#4569))
* Remove shared helper from inset text component ([PR #4571](#4571))
* Use component wrapper on contextual footer ([PR #4562](#4562))
* Update Govspeak "Warning Text" component styles ([PR #4487](#4487))
* Make "Add another" component styles more specific ([PR #4579](#4579))
* Translate "and" connective in metadata component to Chinese, Russian and Arabic ([PR #4580](#4580))
@unoduetre unoduetre mentioned this pull request Jan 22, 2025
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