-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat: ClampedText component #28664
feat: ClampedText component #28664
Conversation
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.
PR Summary
Added a new ClampedText component that provides text truncation with "Show more"/"Show less" functionality, primarily used for error tracking issue descriptions.
- Implemented CSS line clamping in
/frontend/src/lib/lemon-ui/ClampedText/ClampedText.tsx
using WebkitLineClamp for efficient text truncation - Added smart detection of text overflow using
scrollHeight > clientHeight
comparison to only show toggle when needed - Integrated component in
/frontend/src/scenes/error-tracking/issue/Metadata.tsx
, replacing static line-clamp CSS with interactive 2-line truncation - Added comprehensive Storybook examples in
ClampedText.stories.tsx
demonstrating single and multi-line use cases
5 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
const handleToggleShowMore = (show: boolean): void => { | ||
setShowMore(!showMore) | ||
setIsExpanded(!isExpanded) | ||
setLocalLines(show ? undefined : lines) | ||
} |
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.
logic: handleToggleShowMore's 'show' parameter is unused - the function toggles based on !showMore and !isExpanded instead. This could lead to unexpected behavior if the passed show value doesn't match the current state.
const handleToggleShowMore = (show: boolean): void => { | |
setShowMore(!showMore) | |
setIsExpanded(!isExpanded) | |
setLocalLines(show ? undefined : lines) | |
} | |
const handleToggleShowMore = (show: boolean): void => { | |
setShowMore(show) | |
setIsExpanded(show) | |
setLocalLines(show ? undefined : lines) | |
} |
if (isCssEllipsisApplied(elem)) { | ||
if (!(showMore && isExpanded)) { | ||
setShowMore(true) | ||
} | ||
} else { |
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.
style: The nested condition !(showMore && isExpanded)
could be simplified to !showMore || !isExpanded
for better readability and to avoid potential state mismatches
@@ -32,7 +33,7 @@ export const Metadata = (): JSX.Element => { | |||
|
|||
return ( | |||
<div className="space-y-1"> | |||
{issue ? <div className="italic line-clamp-3">{issue.description}</div> : <LemonSkeleton />} | |||
{issue && issue.description ? <ClampedText text={issue.description} lines={2} /> : <LemonSkeleton />} |
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.
style: Consider handling empty string case - currently shows skeleton for null/undefined but renders empty ClampedText for ''
{issue && issue.description ? <ClampedText text={issue.description} lines={2} /> : <LemonSkeleton />} | |
{issue && issue.description?.trim() ? <ClampedText text={issue.description} lines={2} /> : <LemonSkeleton />} |
const MediaTemplate: StoryFn<typeof ClampedText> = (props: ClampedTextProps) => { | ||
return <ClampedText {...props} /> |
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.
style: Template name 'MediaTemplate' seems incorrect since this component handles text, not media
Size Change: -5 B (0%) Total Size: 1.18 MB ℹ️ View Unchanged
|
Problem
Would be nice for the description to expand beyond a certain point
Changes
Added a new ClampedText component