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

COM-162: Add EllipsisTooltip component to admin #1548

Closed
wants to merge 1 commit into from

Conversation

jamesricky
Copy link
Contributor

@jamesricky jamesricky commented Jan 9, 2024

Used to automatically add a tooltip to text that is too long to fit in its container.
This is useful for displaying text in a table or data grid when the text might be too long to fit in the column.

Theming-Support is added in a separate PR (#1551 ) because of the theming-refactor (#1376).

@jamesricky jamesricky self-assigned this Jan 9, 2024
@auto-assign auto-assign bot requested a review from johnnyomair January 9, 2024 12:40
@jamesricky jamesricky removed the request for review from johnnyomair January 9, 2024 14:00
@jamesricky jamesricky marked this pull request as draft January 9, 2024 14:09
@jamesricky jamesricky changed the base branch from feature/refactor-admin-component-theming to main January 9, 2024 15:35
@jamesricky jamesricky force-pushed the ellipsis-tooltip branch 2 times, most recently from f1f61cd to c7442a0 Compare January 10, 2024 06:17
@jamesricky jamesricky marked this pull request as ready for review January 10, 2024 07:26
@auto-assign auto-assign bot requested a review from johnnyomair January 10, 2024 07:26
Copy link
Collaborator

@johnnyomair johnnyomair left a comment

Choose a reason for hiding this comment

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

I'm unsure about the name of the component. Ellipsis is a technical term for overflowing text content, but the content of this component could be anything, right? Maybe we could use a more generic name, like <Overflowable>, HandleOverflow or similar? 🤔

Comment on lines +57 to +63
{renderWithTooltip ? (
<Tooltip PopperProps={{ anchorEl: rootRef.current }} title={children}>
{content}
</Tooltip>
) : (
content
)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of this conditional render you could render the Tooltip all the time and use undefined for title if the content fits into the container. I believe this would simplify the ref handling

Comment on lines +15 to +21
const updateRenderWithTooltip = React.useCallback(() => {
if (rootRef.current && contentRef.current) {
setRenderWithTooltip(contentRef.current.offsetWidth > rootRef.current.clientWidth);
}
// The dependency array items must be `.current`, otherwise the callback will not be called, when the html-element is rendered.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [rootRef.current, contentRef.current]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could define the function inside the useEffect for simplicity. Also, if you always render the tooltip, the refs wouldn't need to be in the dependency array

@jamesricky jamesricky marked this pull request as draft January 24, 2024 09:47
@jamesricky jamesricky closed this Jan 31, 2024
@johnnyomair johnnyomair deleted the ellipsis-tooltip branch July 24, 2024 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants