DSD-1585: HelperErrorText styling bug

[oliviawongnyc]

Fixes JIRA ticket DSD-1585

This PR does the following:

• Fixes issue with HelperErrorText where, if text value was not a string, necessary styles weren't applied.
• Important note!! I realize my change broke some functionality, but I wonder if we are ok with it? If so, I will update the documentation.

Basically, the following test failed because the code now sees strings as strings and JSX elements as JSX elements, rather than interpreting HTML strings like "This text is bold". (Marty this is why we were using dangerouslySetInnerHtml, I believe), but I feel like this maybe should be the case anyway. Let me know what you think.

it("renders the text passed as an HTML string", () => { render(); expect(screen.getByText("This text is bold")).toBeInTheDocument(); });

How has this been tested?

• Locally, in Storybook.

Accessibility concerns or updates

• None.

Accessibility Checklist

• [ ] Checked Storybook's "Accessibility" tab for color contrast and other issues.
• [ ] The feature works with keyboard inputs including tabbing back and forward and pressing space, enter, arrow, and esc keys.
• [ ] For hidden text or when aria-live is used, a screenreader was used to verify the text is read.
• [ ] For features that involve UI updates and focusing on DOM refs, focus management was reviewed.
• [ ] The feature works when the page is zoomed in to 200% and 400%.

Open Questions

-

Checklist:

• [ ] I have updated the Storybook documentation and changelog accordingly.
• [ ] I have added relevant accessibility documentation for this pull request.
• [ ] All new and existing tests passed.

Front End Review:

• [ ] Review the Vercel preview deployment once it is ready.

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
nypl-design-system	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 17, 2024 6:10pm

[bigfishdesign13]

With this update, when the text prop is passed as a string, HTML tags that are included in the string will no longer be rendered. That is definitely a breaking change and it will either need to be reverted or annother solution will need to be implemented to allow the HTML tags to be rendered propperly. Form field helper text is everywhere and we can't take this risk.

I see that we are using dangerouslySetInnerHTML in a number of other components, so I think we can stick with that.

Thoughts?

[EdwinGuzman]

This very closely follows the requirement in the ticket to remove the conditional logic but I still think it's needed. We just recently added this to the Banner component because of the use-case where HTML is stored as a string from either a database, API call, or environment variable.

I think the conditional should still be there so it's backwards compatible, but wrap the text in <Box __css={styles.innerChild}>{text}</Box>. The issue might also come from how the parent Box wrapper is styled.

If the user passes in HTML that has top margin or padding, the text will be off and we cannot help that. So maybe add a warning against doing so or being careful about what gets passed -- the simpler the better.