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

Truncate component breaks JSX conventions #3311

Open
bradenmacdonald opened this issue Dec 3, 2024 · 3 comments
Open

Truncate component breaks JSX conventions #3311

bradenmacdonald opened this issue Dec 3, 2024 · 3 comments
Labels
bug Report of or fix for something that isn't working as intended

Comments

@bradenmacdonald
Copy link
Contributor

Feedback summary

The <Truncate> component documentation says:

Note: Truncate supports only plain HTML children and not jsx.

And a supposed example of this can be seen in the "HTML markdown support" section.

However, this is not true. A more accurate statement would be "Truncate supports only a buggy, idiosyncratic version of JSX."

It is JSX, not HTML

First of all, if it was truly HTML instead of JSX, it would have to be passed into the Truncate component as a string; it is in fact being parsed by React's JSX parser. You can clearly see this if you use other JSX syntax like {"string interpolation"}:

Screenshot 2024-12-03 at 10 35 15 AM

Bugs

Second, of all, it throws errors on things that are perfectly valid HTML or JSX:

Screenshot 2024-12-03 at 10 37 43 AM

Screenshot 2024-12-03 at 10 38 01 AM

Incompatibility

Finally, because this is actually JSX but it's not implemented correctly, TypeScript is always going to throw errors like this (which is how I discovered all this):

Type '{ children: Element; class: string; }' is not assignable to type 'DetailedHTMLProps<HTMLAttributes<HTMLElement>, HTMLElement>'.
  Property 'class' does not exist on type 'DetailedHTMLProps<HTMLAttributes<HTMLElement>, HTMLElement>'. Did you mean 'className'?

I don't think it is possible to tell TypeScript that some portion of the JSX is using our own special version of JSX and should be using class instead of className.

Suggestion

At the very least, I suggest fixing the bugs and implementing support for className instead of class, htmlFor instead of for, etc. so that we can use this with TypeScript.

But now might be the time to replace the current complex JS implementation with a simple CSS implementation, given that all major browsers have supported -webkit-line-clamp for a while (click "Usage Relative").

@bradenmacdonald bradenmacdonald added the bug Report of or fix for something that isn't working as intended label Dec 3, 2024
@brian-smith-tcril
Copy link
Contributor

After discussing in today's Paragon WG meeting, a reasonable path forward would be to deprecate the current Truncate component (move it to Truncate.Deprecated) and build a new Truncate component that supports our use cases (including a11y concerns around aria-label/title etc.) in as modern a way as possible.

@bradenmacdonald
Copy link
Contributor Author

supports our use cases (including a11y concerns around aria-label/title etc.)

@brian-smith-tcril Are those requirements documented anywhere? I don't see them in the README nor the code.

@kblemel
Copy link

kblemel commented Dec 11, 2024

@brian-smith-tcril We discussed a few top level a11y concerns (e.g., using proper HTML/ARIA semantics and providing equivalent access for keyboard and touchscreen to the full title display on mouseover).

After reviewing WCAG Success Criteria, I realized that I'll need more information on expected use cases to understand possible additional issues with both SC 1.4.10 Reflow (added in 2.1) and 1.4.4 Resize Text.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Report of or fix for something that isn't working as intended
Projects
Status: Backlog
Development

No branches or pull requests

3 participants