-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Listview display none #6357
Listview display none #6357
Conversation
GET_BUILD |
## API Changes
unknown top level export { type: 'any' } |
@@ -205,7 +205,7 @@ export class ListLayout<T> extends Layout<Node<T>> implements KeyboardDelegate, | |||
|
|||
let layoutNode = this.buildChild(node, 0, y); | |||
y = layoutNode.layoutInfo.rect.maxY; | |||
nodes.push(layoutNode); | |||
this.validRect.height && nodes.push(layoutNode); |
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.
what is the height at this point? 0? undefined?
I think it'd be better to be a little more explicit and check if it's > 0
, and if it's undefined, it'd be good to add a !isNaN
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.
If I go into the storybook controls and enable the isQuiet prop, this breaks, which makes me think this won't work beyond the first render. We may need to look a little closer at how to handle this.
@@ -383,6 +382,7 @@ export class ListLayout<T> extends Layout<Node<T>> implements KeyboardDelegate, | |||
} | |||
|
|||
updateItemSize(key: Key, size: Size) { | |||
if (size.height === 0) {return false;} |
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.
It's not clear if it is a safe change. Probably the height of a list item is 0 only if display:none
set.
On the other hand if somehow the item size is small enough (say 1), react will throw the same exception even if the ListView is visible. This is because after render the size is calculated (1 in this case) and a re-render will add one more item to the visible item's list, which will notice it's size and render again... If the collection is large enough react will treat it as an infinite recursion.
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.
The page does not crash if padding is set to 1 on ListLayout, but this is not configurable. Can be verified by setting the padding in ListView.tsx.
Closing as resolved by #6497 |
Closes #6298
✅ Pull Request Checklist:
📝 Test Instructions:
ListView Story "display: none with many items" does not crash
🧢 Your Project: