[ActionList] Prevent title="[object Object]" when Description has non-string children #6223
Conversation
🦋 Changeset detectedLatest commit: 4be97cb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
integration-tests: recommended
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the ActionList.Description component by adding a title prop and automatically applying a tooltip for truncated text when the content is a string.
- Introduces a new
titleprop and computes aneffectiveTitlefallback for string children - Renders the
titleattribute on the truncated (<Truncate>) variant usingeffectiveTitle - Adds tests and a story example to validate the new tooltip behavior
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/react/src/ActionList/Description.tsx | Added title prop, effectiveTitle logic, and updated truncated render |
| packages/react/src/ActionList/ActionList.test.tsx | New tests verifying title attribute for string, complex, and custom cases |
| packages/react/src/ActionList/ActionList.features.stories.tsx | Storybook example illustrating the title prop on truncated content |
| .changeset/brown-rules-taste.md | Recorded patch release changelog |
Comments suppressed due to low confidence (1)
packages/react/src/ActionList/ActionList.test.tsx:178
- Add a test case for when
title=""is provided on a truncated string child to ensure that an explicit empty title is preserved and not overridden by the auto-fallback logic.
it('sets title correctly for Description component', () => {
size-limit report 📦
|
|
🟢 golden-jobs completed with status |
github-actions
bot
added
integration-tests: passing
| /** | ||
| * The title attribute for the truncated text tooltip. | ||
| * If not provided and children is a string, it will be set automatically. | ||
| */ |
There was a problem hiding this comment.
Can we add an additional note here for accessibility? The title attribute is a tricky one since it's not really accessible, but since we're utilizing it anyways, we can try suggest only utilizing it when it's truly needed 🤔
| /** | |
| * The title attribute for the truncated text tooltip. | |
| * If not provided and children is a string, it will be set automatically. | |
| */ | |
| /** | |
| * The title attribute for the truncated text tooltip. | |
| * If not provided and children is a string, it will be set automatically. | |
| * | |
| * `title` should be used sparingly, as it may be inaccessible to some users. | |
| */ |
| ...props | ||
| }) => { | ||
| const {blockDescriptionId, inlineDescriptionId} = React.useContext(ItemContext) | ||
| const effectiveTitle = title || (typeof props.children === 'string' ? props.children : '') |
There was a problem hiding this comment.
Nit: Should we add additional types so that title can only be used when variant !== 'block' or truncate === true? Something like this:
type VariantTypes =
| {
variant: 'block'
title: never
}
| {
/**
* Secondary text style variations.
*
* - `"inline"` - Secondary text is positioned beside primary text.
* - `"block"` - Secondary text is positioned below primary text.
*/
variant?: 'inline'
/**
* The title attribute for the truncated text tooltip.
* If not provided and children is a string, it will be set automatically.
*
* `title` should be used sparingly, as it may be inaccessible to some users.
*/
title?: string
}
export type ActionListDescriptionProps = VariantTypes & { ... }Not a blocker, just an idea! 😁 Let me know what you think!
There was a problem hiding this comment.
Great suggestion! 🎉 I've added the DescriptionVariantTypes.
github-actions
bot
added
the
integration-tests: recommended
|
👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks! |
|
👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks! |
|
👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks! |
| <ActionList.Description truncate title="Custom title"> | ||
| <span>Complex</span> content | ||
| </ActionList.Description> |
There was a problem hiding this comment.
@TylerJDev would this be considered failing: https://dequeuniversity.com/rules/axe/4.7/label-content-name-mismatch ? 🤔 I don't think axe flags it but was curious if that was the case.
If so, it would be great to derive title from props.children instead of having it be explicit just to avoid the mismatch here when using downstream. In the props.children is not a string case, maybe we could use the text content from the rendered content? Just thinking out loud, curious what you think @liuliu-dev and @TylerJDev 👀
There was a problem hiding this comment.
I think as long as the text is provided, title will default to the description, rather than accessible name. If we were able to utilize Tooltip instead of rely on the title "tooltip", it would be best, but that'd be harder to implement 😅
In the props.children is not a string case, maybe we could use the text content from the rendered content?
For this, would we need to grab the text after it was rendered? (e.g. accessing the DOM element's text content within the description container)
github-actions
bot
removed
the
integration-tests: recommended
|
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/386228 |
Closes #4400
Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist