Skip to content

ref(container) add ellipsis support #105956

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

Closed
JonasBa wants to merge 2 commits into from
Closed

ref(container) add ellipsis support #105956

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4c045d8
ref(container) add ellipsis support
JonasBa Jan 8, 2026
8250269
add docs
JonasBa Jan 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions static/app/components/core/layout/container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@ import {

/* eslint-disable typescript-sort-keys/interface */
interface ContainerLayoutProps {
/**
* When true, overflow is set to hidden, text-overflow is set to ellipsis, and white-space is set to nowrap.
* Individual properties can be overridden by setting the corresponding property (e.g. overflow, text-overflow, white-space).
* @default undefined
*/
ellipsis?: Responsive<boolean>;

background?: Responsive<SurfaceVariant>;
display?: Responsive<
| 'block'
Expand Down Expand Up @@ -82,6 +89,9 @@ interface ContainerLayoutProps {
alignSelf?: Responsive<React.CSSProperties['alignSelf']>;
justifySelf?: Responsive<React.CSSProperties['justifySelf']>;

textOverflow?: Responsive<React.CSSProperties['textOverflow']>;
whiteSpace?: Responsive<React.CSSProperties['whiteSpace']>;

/**
* Prefer using Flex or Grid gap as opposed to margin.
* @deprecated
Expand Down Expand Up @@ -167,6 +177,7 @@ const omitContainerProps = new Set<keyof ContainerLayoutProps | 'as'>([
'bottom',
'column',
'display',
'ellipsis',
'flex',
'flexBasis',
'flexGrow',
Expand Down Expand Up @@ -197,8 +208,10 @@ const omitContainerProps = new Set<keyof ContainerLayoutProps | 'as'>([
'radius',
'right',
'row',
'textOverflow',
'top',
'width',
'whiteSpace',
Copy link
Collaborator

@TkDodo TkDodo Jan 9, 2026

Choose a reason for hiding this comment

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

On Text, we have this named wrap:

sentry/static/app/components/core/text/text.tsx

Line 139 in 776966b

white-space: ${p => (p.wrap ? p.wrap : p.ellipsis ? 'nowrap' : undefined)};

The more I think about it, the more I would like us to stay as close as possible to html attributes. For example, <Text bold> isn’t better than setting font-weight and <Text wrap=“nowrap”> isn’t better than <Text whiteSpace=“nowrap”>, especially if it works differently on Container and we also have a textWrap prop on Text.

I sometimes find myself looking at the implementation of our core components just to know which props exist because I can’t find what I’m looking for in auto complete. That’s partially because auto complete lists all html props so it’s not helpful, but also because I type fontWeight or textDecoration and don’t find anything because our props are named differently ...

Copy link
Member Author

@JonasBa JonasBa Jan 9, 2026

Choose a reason for hiding this comment

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

That's fair feedback. I think we can pull usage and see if that sticks, or if it's not been a good addition.

]);

export const Container = styled(
Expand Down Expand Up @@ -277,6 +290,19 @@ export const Container = styled(
${p => rc('border-left', p.borderLeft, p.theme, getBorder)};
${p => rc('border-right', p.borderRight, p.theme, getBorder)};

${p =>
rc(
'text-overflow',
p.textOverflow ? p.textOverflow : p.ellipsis ? 'ellipsis' : undefined,
p.theme
)};
${p =>
rc(
'white-space',
p.whiteSpace ? p.whiteSpace : p.ellipsis ? 'nowrap' : undefined,
p.theme
)};
Copy link
Contributor

@cursor cursor bot Jan 8, 2026

Choose a reason for hiding this comment

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

Responsive ellipsis values not correctly mapped to CSS

Medium Severity

The ellipsis prop is typed as Responsive<boolean>, allowing responsive objects like {base: true, md: false}. However, the implementation uses simple truthiness checks (p.ellipsis ? 'ellipsis' : undefined) which treat any object as truthy. When a responsive object is passed, it results in 'ellipsis' being applied unconditionally instead of mapping each breakpoint's boolean value to the corresponding CSS value. The responsive behavior is lost.

Fix in Cursor Fix in Web

Comment on lines +299 to +309

This comment was marked as outdated.

Sign in to view

Comment on lines +303 to +309
Copy link

@sentry sentry bot Jan 8, 2026

Choose a reason for hiding this comment

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

Bug: The text-overflow and white-space properties incorrectly use a ternary on the ellipsis prop. This bypasses responsive handling when ellipsis is an object, applying styles globally.
Severity: MEDIUM

🔍 Detailed Analysis

When the ellipsis prop is a responsive object (e.g., {xs: true, md: false}), the ternary expressions p.ellipsis ? 'ellipsis' : undefined and p.ellipsis ? 'nowrap' : undefined evaluate the object as truthy. This causes them to immediately return the string values 'ellipsis' or 'nowrap', bypassing the responsive styling function rc(). Consequently, text-overflow and white-space are applied globally across all breakpoints, contrary to the intended responsive behavior which is correctly implemented for the overflow property. This creates an inconsistent and buggy API for a newly introduced feature.

💡 Suggested Fix

Refactor the text-overflow and white-space style definitions to use the rc() utility function directly with a resolver, similar to how the overflow property is handled. This will ensure that when p.ellipsis is a responsive object, the styles are generated with the correct media queries. Avoid using a simple truthiness check on the p.ellipsis prop.

🤖 Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: static/app/components/core/layout/container.tsx#L303-L309

Potential issue: When the `ellipsis` prop is a responsive object (e.g., `{xs: true, md:
false}`), the ternary expressions `p.ellipsis ? 'ellipsis' : undefined` and `p.ellipsis
? 'nowrap' : undefined` evaluate the object as truthy. This causes them to immediately
return the string values `'ellipsis'` or `'nowrap'`, bypassing the responsive styling
function `rc()`. Consequently, `text-overflow` and `white-space` are applied globally
across all breakpoints, contrary to the intended responsive behavior which is correctly
implemented for the `overflow` property. This creates an inconsistent and buggy API for
a newly introduced feature.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8369330


/**
* This cast is required because styled-components does not preserve the generic signature of the wrapped component.
* By default, the generic type parameter <T> is lost, so we use 'as unknown as' to restore the correct typing.
Expand Down
Loading