Repository navigation
feat: support description text in tree items #10710
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
6bed189
bb0783f
8463992
ae4b5d3
d4330f8
3a5394a
b0b3d28
553bd0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,7 +107,7 @@ export function useGridListItem<T>( | |
| let {direction} = useLocale(); | ||
| let {onAction, linkBehavior, keyboardNavigationBehavior, shouldSelectOnPressUp} = | ||
| listMap.get(state)!; | ||
| let descriptionId = useSlotId(); | ||
| let descriptionId = useSlotId([node]); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume this is related to this test?: If so, was this an existing bug in GridList? and can you add a test specifically for GridList. It can be basically the same as the Tree one. That way we can verify before and after
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would node.key (or id? i'm going off memory for this comment) be better?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't forget to answer in comment thread
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, GridList had the same stale
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but won't the node change on pretty much every render? I think they are immutable if i recall correctly, so this could be a lot of re-renders?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You were right: with no description, changing an unrelated item prop caused two extra Added regressions for Tree and GridList. Full unit, SSR and browser suites pass; the browser run needed retries for intermittent WebKit failures in other components.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If someone is using the renderProps children as a function won't that cause new children every render as well?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right. Inline functions caused extra updates, and stable functions missed descriptions added on selection. The slot now keeps its ID and updates presence only when it changes, which also preserves React 18 focus restoration. Added both render-function cases for Tree and GridList; the full unit suites on React 18 and 19, SSR and browser checks pass. |
||
|
|
||
| // We need to track the key of the item at the time it was last focused so that we force | ||
| // focus to go to the item when the DOM node is reused for a different item in a virtualizer. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,6 +81,14 @@ export function useTreeItem<T>( | |
| // TODO: should it return a state specifically for isExpanded? Or is aria attribute sufficient? | ||
| return { | ||
| ...gridListAria, | ||
| rowProps: { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume you don't need this override. We can just inherit the behavior from |
||
| ...gridListAria.rowProps, | ||
| 'aria-labelledby': undefined, | ||
| 'aria-describedby': | ||
| [node.props['aria-describedby'], gridListAria.descriptionProps.id] | ||
| .filter(Boolean) | ||
| .join(' ') || undefined | ||
| }, | ||
| expandButtonProps | ||
| }; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.