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

fix(ActionList): do not truncate description by default #5169

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
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
5 changes: 5 additions & 0 deletions .changeset/cyan-lions-itch.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": minor
---

fix(ActionList): do not truncate description by default
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
6 changes: 6 additions & 0 deletions packages/react/src/ActionList/ActionList.docs.json
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,12 @@
"type": "string | undefined",
"defaultValue": "",
"description": "CSS string"
},
{
"name": "truncate",
"type": "boolean",
"defaultValue": "false",
"description": "Whether the inline description should truncate the text on overflow."
}
]
},
Expand Down
14 changes: 13 additions & 1 deletion packages/react/src/ActionList/ActionList.features.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,19 @@ export const TextWrapAndTruncation = () => (
<ArrowRightIcon />
</ActionList.LeadingVisual>
Inline Description
<ActionList.Description>This description gets truncated because it is inline</ActionList.Description>
<ActionList.Description truncate>
This description gets truncated because it is inline with truncation
</ActionList.Description>
<ActionList.TrailingVisual>
<ArrowLeftIcon />
</ActionList.TrailingVisual>
</ActionList.Item>
<ActionList.Item>
<ActionList.LeadingVisual>
<ArrowRightIcon />
</ActionList.LeadingVisual>
Inline Description
<ActionList.Description>This description wraps because it is inline without truncation</ActionList.Description>
<ActionList.TrailingVisual>
<ArrowLeftIcon />
</ActionList.TrailingVisual>
Expand Down
48 changes: 48 additions & 0 deletions packages/react/src/ActionList/ActionList.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -648,4 +648,52 @@ describe('ActionList', () => {
await userEvent.keyboard('{ArrowUp}')
expect(document.activeElement).toHaveTextContent('Option 4')
})

describe('ActionList.Description', () => {
it('should render the description as inline without truncation by default', () => {
const {getByText} = HTMLRender(
<ActionList>
<ActionList.Item>
Item 1<ActionList.Description>Item 1 description</ActionList.Description>
</ActionList.Item>
</ActionList>,
)

const description = getByText('Item 1 description')
expect(description.tagName).toBe('SPAN')
expect(description).toHaveStyleRule('flex-basis', 'auto')
expect(description).not.toHaveStyleRule('overflow', 'ellipsis')
expect(description).not.toHaveStyleRule('white-space', 'nowrap')
})
it('should render the description as `Truncate` when truncate is true', () => {
const {getByText} = HTMLRender(
<ActionList>
<ActionList.Item>
Item 1<ActionList.Description truncate>Item 1 description</ActionList.Description>
</ActionList.Item>
</ActionList>,
)

const description = getByText('Item 1 description')
expect(description.tagName).toBe('DIV')
expect(description).toHaveAttribute('title', 'Item 1 description')
expect(description).toHaveStyleRule('flex-basis', '0')
expect(description).toHaveStyleRule('text-overflow', 'ellipsis')
expect(description).toHaveStyleRule('overflow', 'hidden')
expect(description).toHaveStyleRule('white-space', 'nowrap')
Copy link
Member

Choose a reason for hiding this comment

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

Very detail tests!! Love it 🔥

I’m also curious to learn when you prefer to use Jest for visual style testing versus a tool like Playwright. 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a great question! I actually didn't take the fact that we already had a playwright test for the truncation story into account when writing these so this might be a bit "overkill" haha. I think I tend to go for Jest because it's what I'm most familiar with but love how "complete" vrt testing is in that it'll fail on the smallest visual change in the entire story so it feels like it covers more ground than a specific jest test. That being said I'm a big fan of redundancy.

Copy link
Member

Choose a reason for hiding this comment

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

love how "complete" vrt testing is in that it'll fail on the smallest visual change in the entire story so it feels like it covers more ground than a specific jest test.

Love this!

})
it('should render the description in a new line when variant is block', () => {
const {getByText} = HTMLRender(
<ActionList>
<ActionList.Item>
Item 1<ActionList.Description variant="block">Item 1 description</ActionList.Description>
</ActionList.Item>
</ActionList>,
)

const description = getByText('Item 1 description')
expect(description.tagName).toBe('SPAN')
expect(description.parentElement).toHaveAttribute('data-component', 'ActionList.Item--DividerContainer')
})
})
})
11 changes: 8 additions & 3 deletions packages/react/src/ActionList/Description.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,24 @@ export type ActionListDescriptionProps = {
*/
variant?: 'inline' | 'block'
className?: string
/**
* Whether the inline description should truncate the text on overflow.
*/
truncate?: boolean
} & SxProp

export const Description: React.FC<React.PropsWithChildren<ActionListDescriptionProps>> = ({
variant = 'inline',
sx = {},
className,
truncate,
...props
}) => {
const styles = {
fontSize: 0,
lineHeight: '16px',
flexGrow: 1,
flexBasis: 0,
flexBasis: variant === 'inline' && !truncate ? 'auto' : 0,
minWidth: 0,
marginLeft: variant === 'block' ? 0 : 2,
color: 'fg.muted',
Expand All @@ -37,11 +42,11 @@ export const Description: React.FC<React.PropsWithChildren<ActionListDescription

const {blockDescriptionId, inlineDescriptionId} = React.useContext(ItemContext)

return variant === 'block' ? (
return variant === 'block' || !truncate ? (
<Box
as="span"
sx={merge(styles, sx as SxProp)}
id={blockDescriptionId}
id={variant === 'block' ? blockDescriptionId : inlineDescriptionId}
Copy link
Member

Choose a reason for hiding this comment

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

I didn't know we have different IDs for inline and block 💭 good to know

Copy link
Member Author

Choose a reason for hiding this comment

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

was surprised by this one as well hahaah

className={className}
data-component="ActionList.Description"
>
Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/ActionList/Item.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
flexGrow: slots.inlineDescription ? 0 : 1,
fontWeight: slots.inlineDescription || slots.blockDescription || active ? 'bold' : 'normal',
marginBlockEnd: slots.blockDescription ? '4px' : undefined,
wordBreak: 'break-word',
wordBreak: slots.inlineDescription ? 'normal' : 'break-word',
}}
>
{childrenWithoutSlots}
Expand Down
Loading