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

Tag - Text truncation for overflow fix #2655

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

dchyun
Copy link
Contributor

@dchyun dchyun commented Jan 17, 2025

📌 Summary

This PR fixes an overflow issue present in the Tag when text wraps to > 3 lines. It implements the intended design change to address this, which truncates any text that goes to multiple lines. When truncation occurs, an ellipsis is rendered, and a tooltip showing the full text is added.

It also adds width: fit-content; to the tag to ensure a tag's width never goes beyond its content.

🛠️ Detailed description

It was observed in the Tag in HDS-3854 that when the text exceeds 3 lines, the background of the dismiss button obscures the border of the element.

Per design guidance, the chosen solution has been to truncate any text in the tag that goes beyond one line, and add an ellipsis and tooltip with the full text when this truncation occurs.

The text is truncated when the text would wrap to multiple lines using overflow: hidden and -webkit-line-clamp: 1. When this occurs a tooltip is also added using the hds-tooltip modifier, and the Hds::TooltipButton.

There are several other changes related to this fix.

  • The max-width of the text is set to 150px
  • A new property @tooltipPlacement has been added to allow for customization of the tooltip placement
  • width: fit-content has been added to prevent the tag width from ever exceeding its content. This was present previously when the tag had a parent with display: grid.

📸 Screenshots

Text Truncation

Before
Screenshot 2025-01-24 at 9 24 43 AM

After
Screenshot 2025-02-10 at 4 27 06 PM

After - Hover / Focus
Screenshot 2025-02-10 at 4 25 52 PM

Width

Before
Screenshot 2025-02-05 at 9 28 22 AM

After
Screenshot 2025-02-10 at 4 26 07 PM

🔗 External links

Jira ticket: HDS-4317
Figma file: Link


👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

Copy link

vercel bot commented Jan 17, 2025

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

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Feb 11, 2025 8:07pm
hds-website ✅ Ready (Inspect) Visit Preview Feb 11, 2025 8:07pm

Copy link
Contributor

@majedelass majedelass left a comment

Choose a reason for hiding this comment

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

This is looking great, although I did find an interesting "bug"
image

The tooltip is still there even if the text is fully revealed. Wonder if there is a way to fix this?

UPDATE: It looks like something that was once truncated, but then isn't will have a tooltip. We should also check screen reader behavior. Does the text get read out twice or just once? I think we can loop in the entire team for A11Y

@dchyun
Copy link
Contributor Author

dchyun commented Jan 21, 2025

This is looking great, although I did find an interesting "bug" image

The tooltip is still there even if the text is fully revealed. Wonder if there is a way to fix this?

Did you notice this after doing some resizing of the page, or on load? One open issue with the fix is that the tooltip will be added if the text goes from not cut off to cut off on a resize, but not removed if the text goes from cut off to shown.

I figure this isn't a use case that will come up often as users aren't usually resizing their browser back and forth, but will see if I can find some fix.

Update: This edge case issue with resizing has been resolved

@MelSumner
Copy link
Contributor

Def want to call out that the there needs to be documentation that informs consumers that truncated text is not accessible for all users and they should prefer limiting the text length before the need for truncation exists.

@majedelass
Copy link
Contributor

majedelass commented Jan 24, 2025

Def want to call out that the there needs to be documentation that informs consumers that truncated text is not accessible for all users and they should prefer limiting the text length before the need for truncation exists.

Can you elaborate on this? Truncated text will always have tooltips associated to it. Also from my understanding, truncation is a CSS based visual indicator that should still be accessible by a screen reader. One call out is to make sure that the text isn't read out twice. Is there something else I'm missing?

@dchyun
Copy link
Contributor Author

dchyun commented Jan 24, 2025

Def want to call out that the there needs to be documentation that informs consumers that truncated text is not accessible for all users and they should prefer limiting the text length before the need for truncation exists.

Can you elaborate on this? Truncated text will always have tooltips associated to it. Also from my understanding, truncation is a CSS based visual indicator that should still be accessible by a screen reader. One call out is to make sure that the text isn't read out twice. Is there something else I'm missing?

Yes the truncation is only via CSS. In the DOM the text will still be un-truncated. I did test with voiceover and this does mean that the text does get read twice, which isn't ideal, but no content is lost. It reads the text of the button and then the tooltip associated with it.

Screenshot 2025-01-24 at 10 04 31 AM Screenshot 2025-01-24 at 10 07 01 AM

@dchyun dchyun marked this pull request as ready for review January 24, 2025 15:18
@dchyun dchyun requested a review from a team as a code owner January 24, 2025 15:18
@dchyun dchyun mentioned this pull request Feb 6, 2025
2 tasks
alex-ju
alex-ju previously approved these changes Feb 10, 2025
Copy link
Member

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

Left one suggestion – otherwise looking great!

@tracked private _isTextOverflow!: boolean;
private _observer!: ResizeObserver;

private _setUpObserver = modifier((element: HTMLElement) => {
Copy link
Member

Choose a reason for hiding this comment

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

I like how this aligns with the AdvancedTable observer!

Copy link
Contributor

Choose a reason for hiding this comment

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

all credit to dylan, I followed his pattern :)

@@ -104,4 +125,8 @@ export default class HdsTag extends Component<HdsTagSignature> {

return classes.join(' ');
}

private _isOverflow(el: Element): boolean {
return el.scrollHeight > el.clientHeight;
Copy link
Member

Choose a reason for hiding this comment

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

🙌

alex-ju
alex-ju previously approved these changes Feb 10, 2025
Copy link
Contributor

@shleewhite shleewhite left a comment

Choose a reason for hiding this comment

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

Just a couple small suggestions, looks good!

.changeset/brown-wolves-admire.md Outdated Show resolved Hide resolved
showcase/app/templates/components/tag.hbs Outdated Show resolved Hide resolved
shleewhite
shleewhite previously approved these changes Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants