-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
ref(ui): Refactor EmptyMessage to use core components and simplify API #103798
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
ref(ui): Refactor EmptyMessage to use core components and simplify API #103798
Conversation
| data-test-id="empty-message" | ||
| {...stackProps} | ||
| {...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.
Bug: Invalid HTML nesting of div inside span
The EmptyMessage component uses Text as its root element, which defaults to rendering a span. However, it contains Container children (for the icon and action) that render as divs. Nesting a div inside a span is invalid HTML and can cause hydration mismatches or layout issues. The Text component should use the as="div" prop to render a block element.
| size={size === 'large' ? 'xl' : 'md'} | ||
| data-test-id="empty-message" | ||
| {...stackProps} | ||
| {...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.
Bug: External className overwrites layout styles
The component spreads props after stackProps on the root Text element. stackProps contains the className generated by the Flex component which defines the layout (display, gap, padding). If EmptyMessage is passed a className (e.g. via styled()), it overwrites the stackProps.className, causing the component to lose all its internal layout styling. Class names should be merged.
| )} | ||
| {action && <Container paddingTop="xl">{action}</Container>} | ||
| </Text> | ||
| )} |
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.
Bug: Invalid HTML nesting of div inside span
The EmptyMessage uses Text as its root element, which defaults to rendering a span. However, its children (icon, action) use Container, which renders a div. Nesting block-level elements (div) inside inline elements (span) is invalid HTML and can cause hydration or layout issues. The root Text should be rendered as a block element (e.g., as="div").
JonasBa
left a comment
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.
A couple minor suggestions, but a great pass on improving this, thank you!
6585134 to
e12e851
Compare
7c4faf1 to
ea381b2
Compare
Modernizes the EmptyMessage component and removes redundant description prop: - Refactors EmptyMessage from styled-components to core layout primitives (Flex, Container, Text) - Removes redundant `description` prop - all content now uses `children` - Automatically applies text-wrap: balance to all empty state messages for better readability - Migrates 22 usage sites to use `children` instead of `description` prop - Simplifies component API by having a single way to pass content This reduces the component's surface area and makes it more consistent with the design system's layout and typography primitives.
ea381b2 to
ed19b0a
Compare
| > | ||
| {icon && ( | ||
| <IconDefaultsProvider size="xl"> | ||
| <Container color={theme.isChonk ? theme.gray400 : theme.gray200}> |
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.
Bug: Icon color prop has no effect
The color prop passed to Container won't apply any styling because Container doesn't support a color prop for styling purposes. The prop will be rendered as an HTML attribute but won't affect the icon's color, causing icons to display with default colors instead of the intended gray tones.
| {action && <Container paddingTop="xl">{action}</Container>} | ||
| </Text> | ||
| )} | ||
| </Flex> |
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.
Bug: Flex layout broken by Text display override
The Flex component's render prop passes display: flex styling via className to the Text component, but the Text component sets display: inline-block because of the align="center" prop. This causes a CSS conflict where the Text's display property likely overrides the Flex's display property, breaking the flexbox layout. The intended vertical stacking with gaps between icon, title, children, and action elements won't work correctly.
Modernizes the EmptyMessage component and removes redundant description prop:
descriptionprop - all content now useschildrenchildreninstead ofdescriptionpropThis reduces the component's surface area and makes it more consistent with the design system's layout and typography primitives.