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.

Sign up for GitHub

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

Jump to bottom

add truncation support and tip for long tags #6906

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

add truncation support and tip for long tags #6906

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
59ee7d2
add truncation support and tip for long tags
briancoburn Jul 26, 2023
ecc3468
fixed tests and linting
briancoburn Jul 26, 2023
e0094f5
changed to truncate on width
briancoburn Jul 28, 2023
a0beecb
Merge branch 'master' into master
briancoburn Aug 1, 2023
915ed27
Merge branch 'master' into master
briancoburn Aug 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
58 changes: 52 additions & 6 deletions src/js/components/Tag/Tag.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,25 @@ import { defaultProps } from '../../default-props';
import { TagPropTypes } from './propTypes';
import { Box } from '../Box';
import { Text } from '../Text';
import { Tip } from '../Tip';

import { StyledRemoveButton, StyledTagButton } from './StyledTag';

const Tag = forwardRef(
({ name, value, size, onRemove, onClick, ...rest }, ref) => {
(
{
name,
value,
size,
truncate,
truncateNumChars = 16,
onRemove,
onClick,
...rest
},
ref,
) => {
const theme = useContext(ThemeContext) || defaultProps.theme;

const containerProps = {
ref,
align: 'center',
Expand All @@ -23,27 +35,61 @@ const Tag = forwardRef(
round: theme.tag.size?.[size]?.round || theme.tag.round,
...rest,
};
const contents = (
const displayName =
truncate && name.length > truncateNumChars
? `${name.substring(0, truncateNumChars - 3)}...`
: name;
const displayValue =
truncate && value.length > truncateNumChars
? `${value.substring(0, truncateNumChars - 3)}...`
: value;
Copy link
Collaborator

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.

Copy link
Contributor

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.

Copy link
Author

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.

const tipValue1 = value.length < 128 ? value : value.substring(0, 128);
const tipValue2 = value.length < 128 ? '' : value.substring(128);
const shouldDisplayTip =
truncate &&
(name.length > truncateNumChars || value.length > truncateNumChars);
const tipContent = (
<Box direction="column">
<Text size={size}>{`${name} : `}</Text>
{tipValue1 && (
<Text weight="bold" size={size}>
{tipValue1}
</Text>
)}
{tipValue2 && (
<Text weight="bold" size={size}>
{tipValue2}
</Text>
)}
</Box>
);

const textContent = (
<Box
width={{ min: 'min-content' }}
width={{ min: 'min-content', max: '100%' }}
pad={theme.tag.size?.[size]?.pad || theme.tag.pad}
>
<Text size={size}>
{name && (
<Text {...theme.tag.name} size={size}>
{' '}
{name}
{displayName}
</Text>
)}
{name && value ? <Text size={size}>{theme.tag.separator}</Text> : ''}
{value && (
<Text {...theme.tag.value} size={size}>
{value}
{displayValue}
</Text>
)}
</Text>
</Box>
);
const contents = shouldDisplayTip ? (
<Tip content={tipContent}>{textContent}</Tip>
) : (
textContent
);

if (onClick && onRemove) {
console.warn('Tag cannot combine "onClick" and "onRemove".');
Expand Down
8 changes: 8 additions & 0 deletions src/js/components/Tag/__tests__/__snapshots__/Tag-test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ exports[`Tag basic 1`] = `
-webkit-flex-direction: column;
-ms-flex-direction: column;
flex-direction: column;
max-width: 100%;
min-width: -webkit-min-content;
min-width: -moz-min-content;
min-width: min-content;
Expand Down Expand Up @@ -142,6 +143,7 @@ exports[`Tag onClick 1`] = `
-webkit-flex-direction: column;
-ms-flex-direction: column;
flex-direction: column;
max-width: 100%;
min-width: -webkit-min-content;
min-width: -moz-min-content;
min-width: min-content;
Expand Down Expand Up @@ -358,6 +360,7 @@ exports[`Tag onRemove 1`] = `
-webkit-flex-direction: column;
-ms-flex-direction: column;
flex-direction: column;
max-width: 100%;
min-width: -webkit-min-content;
min-width: -moz-min-content;
min-width: min-content;
Expand Down Expand Up @@ -563,6 +566,7 @@ exports[`Tag size 1`] = `
-webkit-flex-direction: column;
-ms-flex-direction: column;
flex-direction: column;
max-width: 100%;
min-width: -webkit-min-content;
min-width: -moz-min-content;
min-width: min-content;
Expand All @@ -584,6 +588,7 @@ exports[`Tag size 1`] = `
-webkit-flex-direction: column;
-ms-flex-direction: column;
flex-direction: column;
max-width: 100%;
min-width: -webkit-min-content;
min-width: -moz-min-content;
min-width: min-content;
Expand All @@ -605,6 +610,7 @@ exports[`Tag size 1`] = `
-webkit-flex-direction: column;
-ms-flex-direction: column;
flex-direction: column;
max-width: 100%;
min-width: -webkit-min-content;
min-width: -moz-min-content;
min-width: min-content;
Expand All @@ -626,6 +632,7 @@ exports[`Tag size 1`] = `
-webkit-flex-direction: column;
-ms-flex-direction: column;
flex-direction: column;
max-width: 100%;
min-width: -webkit-min-content;
min-width: -moz-min-content;
min-width: min-content;
Expand All @@ -647,6 +654,7 @@ exports[`Tag size 1`] = `
-webkit-flex-direction: column;
-ms-flex-direction: column;
flex-direction: column;
max-width: 100%;
min-width: -webkit-min-content;
min-width: -moz-min-content;
min-width: min-content;
Expand Down