add truncation support and tip for long tags

[briancoburn]

What does this PR do?

HPE Tag spec supports tag names of up to 128 characters and tag values of up to 256 characters. If either value is anywhere near the max length, it can overflow containers and break ui.

This pr adds support for truncation and showing tip for long tag names and values. Addresses this issue:
#6852

Where should the reviewer start?

What testing has been done on this PR?

Tested in ui using the updated grommet tag component. It takes a truncate prop, and optional truncateNumChars prop, which determines the maximum number of characters allowed before truncation occurs.

How should this be manually tested?

Do Jest tests follow these best practices?

• screen is used for querying.
• The correct query is used. (Refer to this list of queries)
• userEvent is used in place of fireEvent.
• asFragment() is used for snapshot testing.

Any background context you want to provide?

What are the relevant issues?

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Should this PR be mentioned in the release notes?

Is this change backwards compatible or is it a breaking change?

[jcfilben]

Hey @briancoburn saw you marked this as a draft, just wanted to check if you are still working on this and we should hold off on reviewing

[MikeKingdom]

FYI I'd rather see this done via some element width constraint and the css ellipsis truncation rather than string truncation based on number of chars.

[samuelgoff]

I agree with @MikeKingdom's feedback. You should be using CSS-only truncation, not substring. In general, if it is possible to accomplish something using CSS-only solutions, you should try to make it work that way.

[briancoburn]

Hey @MikeKingdom and @samuelgoff thanks for the feedback. I have updated the pr to use width based constraints and let the the Text elements handle the truncation internally. Also, tips are "stacked" vertically so they don't get crazy wide and I've added window.resize update for responsive truncation. If you feel like this is ready to go, I'll take it out of draft status, or if you have further feedback, I would be happy to address any other issues.

[jcfilben]

Hi @briancoburn thanks for working on this! Here are some comments so far:

• The tag should wrap by default and only truncate if the truncate prop is set to true or ‘tip’. Currently the tag is truncating by default and no longer wrapping.

• If we have a really long name and a very short value only the name should truncate (or vice versa).

• There is a bit of code that only needs to be run if the truncate prop is true or ‘tip’. Adding a check before this code is run would be beneficial.

• In addition to supporting the truncate prop on Tip we should allow callers to specify tag.name.truncate or tag.value.truncate in the theme since we already document this as a valid option.

• The propTypes.js and index.d.ts files for Tag will need to be updated to reflect the new prop.

• I don’t think we should enforce the MAX_NAME_LENGTH within Grommet. It feels like this is only specific to hpe cases and feel a bit too strict for Grommet.

• Can we add a storybook example that demonstrates the tag truncation?

• Having a padding default value feels a bit brittle. I think there are better ways to determine if the content is overflowing and should be truncated. One potential route you could go is you could prevent the tag from wrapping if truncate is defined and then do a check like containerRef.current.scrollWidth > containerRef.current.offsetWidth.

[MikeKingdom]

In addition to what Jessica mentions, I think instead of a truncation type parameter we should consider a way to just define a max width for the label and value pieces and truncation will be the result. I also was advocating for not actually chopping the label or value strings but instead setting the max-width on the div surrounding them and letting the truncate css do its job if possible. There are some tricks for that css to work in that it needs to be in a sized container

[samuelgoff]

• The tag should wrap by default and only truncate if the truncate prop is set to true or ‘tip’. Currently the tag is truncating by default and no longer wrapping.

Hey @jcfilben while I agree with most of your feedback, I disagree that the tag should wrap by default, because doing so makes it more difficult to read. I would invert the default behavior such that it does not wrap by default, and wrap is an optional prop that can be set to true

[MikeKingdom]

• The tag should wrap by default and only truncate if the truncate prop is set to true or ‘tip’. Currently the tag is truncating by default and no longer wrapping.

Hey @jcfilben while I agree with most of your feedback, I disagree that the tag should wrap by default, because doing so makes it more difficult to read. I would invert the default behavior such that it does not wrap by default, and wrap is an optional prop that can be set to true

I'd agree and even suggest we never try to wrap. It really doesn't do well with the rounded corners.

[jcfilben]

• The tag should wrap by default and only truncate if the truncate prop is set to true or ‘tip’. Currently the tag is truncating by default and no longer wrapping.

Hey @jcfilben while I agree with most of your feedback, I disagree that the tag should wrap by default, because doing so makes it more difficult to read. I would invert the default behavior such that it does not wrap by default, and wrap is an optional prop that can be set to true

I'd agree and even suggest we never try to wrap. It really doesn't do well with the rounded corners.

I think we need to keep the wrap by default for backwards compatibility purposes

[samuelgoff]

I think we need to keep the wrap by default for backwards compatibility purposes

Respectfully, tags that wrap impede readability -- which is the most important criterion for usability of this type of component. If the existing design defaults to wrapping, IMO this is a usability defect & a fix to this defect that breaks backward compatibility is not out of the realm of reason -- especially if this change is clearly communicated in release notes

[jcfilben]

I think we need to keep the wrap by default for backwards compatibility purposes

Respectfully, tags that wrap impede readability -- which is the most important criterion for usability of this type of component. If the existing design defaults to wrapping, IMO this is a usability defect & a fix to this defect that breaks backward compatibility is not out of the realm of reason -- especially if this change is clearly communicated in release notes

That's a fair stance to take, I'm okay with us considering the wrapping to be a defect and make the default behavior truncation. And agree we can just make sure to communicate this in the release notes so that people are not surprised.

[jcfilben]

Hey @briancoburn just wanted to check in on this PR and see if there were any questions or clarifications needed about the review comments

[briancoburn]

hey Jessica- Sorry for the delay. You guys did create some challenging requirements. I hope to have another update for you soon. Brian

On 2023-08-21 08:02, Jessica Filben wrote: Hey @briancoburn [1] just wanted to check in on this PR and see if there were any questions or clarifications needed about the review comments -- Reply to this email directly, view it on GitHub [2], or unsubscribe [3]. You are receiving this because you were mentioned.Message ID: ***@***.***>

Links: ------ [1] https://github.com/briancoburn [2] #6906 (comment) [3] https://github.com/notifications/unsubscribe-auth/AD7V76KIEUHEYZWNIRA3JR3XWNZ7XANCNFSM6AAAAAA2Y6QWEU

[jcfilben]

Hey Brian
No worries, just wanted to check in!