[ActionList] Prevent title="[object Object]" when Description has non-string children #6223
Conversation
🦋 Changeset detectedLatest commit: 6493437 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 📦
|
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)
There was a problem hiding this comment.
Great points! For extracting text from complex children, I think we can probably do this before DOM rendering using a recursive function that traverses props.children. Does this approach make sense?
There was a problem hiding this comment.
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 😅
Totally makes sense 👍
For this, would we need to grab the text after it was rendered?
That was definitely what came to mind for me. Could even be baked into Truncate unless there is a reason to have title not match the contents 🤔 (I'm just not sure at all lol). But definitely get if this seems tedious/requires some setup. One idea could be update Truncate to compute the title if an explicit one is not provided like:
View example
const Truncate = React.forwardRef(function Truncate(
{as, expandable = false, inline = false, maxWidth = 125, title, ...rest},
forwardRef,
) {
const containerRef = useRef<ElementRef<'div'>>(null)
const ref = useMergedRefs(containerRef, forwardRef)
const [computedTitle, setComputedTitle] = useState(title)
useEffect(() => {
if (title) {
return
}
const {current: container} = containerRef
if (!container) {
return
}
const observer = new MutationObserver(() => {
if (container.textContent) {
setComputedTitle(container.textContent.trim())
}
})
observer.observe(container, {
childList: true,
subtree: true,
})
return () => {
observer.disconnect()
}
}, [title])
return (
<StyledTruncate
ref={ref}
as={as}
expandable={expandable}
inline={inline}
maxWidth={maxWidth}
title={computedTitle}
{...rest}
/>
)
}) as PolymorphicForwardRefComponent<'div', TruncateProps>@liuliu-dev mentioned another alternative with children which could definitely work, I'm just personally not sure how to derive the rendered DOM from the element tree in children in order to get the text content.
Separately, I'm assuming with this component that ideally ActionList.Description would work like:
<ActionList.Description truncate>
This is a description
</ActionList.Description>But since children may not always be a string, we want to add title as a way to workaround this. Is that the case? Just wanted to double-check 👀
There was a problem hiding this comment.
That was definitely what came to mind for me. Could even be baked into Truncate unless there is a reason to have title not match the contents 🤔 (I'm just not sure at all lol). But definitely get if this seems tedious/requires some setup.
Yup I agree! I don't think there'd be many cases where a custom implementation would be required. If we could take directly from the text contents then I think that would be preferred.
I feel like even if we did access the DOM directly after render it wouldn't be as bad since the title tooltip won't be rendered until some action is taken (hover), so it's not required instantaneously.
But since children may not always be a string, we want to add title as a way to workaround this. Is that the case? Just wanted to double-check 👀
Yeah! I think that was the main goal, for cases like:
<ActionList.Description truncate>
<>This is a description</>
<ExampleComponentWithExtraText />
</ActionList.Description>I'm thinking that if we could get the text directly regardless of what is passed, then we could potentially omit the prop, unless we want to give more control 🤔
There was a problem hiding this comment.
I agree with both of you, @joshblack and @TylerJDev. Taking the title directly from the text content is the best approach - traversing React nodes to construct the text title is not as reliable. I also agree with @TylerJDev that we might not need the title prop since we can get the text directly from the rendered content.
Thank you all for your inputs!
github-actions
bot
removed
the
integration-tests: recommended
|
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/386228 |
|
🟢 golden-jobs completed with status |
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! |
Closes #4400
Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist