-
Notifications
You must be signed in to change notification settings - Fork 1k
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 truncation support and tip for long tags #6906
Conversation
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 |
src/js/components/Tag/Tag.js
Outdated
const displayName = | ||
truncate && name.length > truncateNumChars | ||
? `${name.substring(0, truncateNumChars - 3)}...` | ||
: name; | ||
const displayValue = | ||
truncate && value.length > truncateNumChars | ||
? `${value.substring(0, truncateNumChars - 3)}...` | ||
: value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
ortag.value.truncate
in the theme since we already document this as a valid option. -
The
propTypes.js
andindex.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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 |
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 |
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. |
Hey @briancoburn just wanted to check in on this PR and see if there were any questions or clarifications needed about the review comments |
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: ***@***.***>
|
Hey Brian |
Closing due to inactivity, feel free to reopen |
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.userEvent
is used in place offireEvent
.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?