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

Pass enableVirtualization flag to itwinui <Tree /> #1133

Merged
merged 5 commits into from
Jan 7, 2025

Conversation

anmolshres98
Copy link
Contributor

Closes #1132

@anmolshres98 anmolshres98 added the tree widget Tree widget or its components related issues label Dec 31, 2024
@anmolshres98 anmolshres98 requested review from a team as code owners December 31, 2024 20:45
@grigasp
Copy link
Member

grigasp commented Jan 6, 2025

@saskliutas, what do you think about this? IMO, our renderer should always use virtualization and the prop should not be available for consumers of our component.

@saskliutas
Copy link
Member

@saskliutas, what do you think about this? IMO, our renderer should always use virtualization and the prop should not be available for consumers of our component.

I think it makes sense to allow consumer to decide whether virtualization is used or not. This TreeRenderer is one of the building blocks we provide so it might be used to build custom tree that do not require virtualization.

@grigasp
Copy link
Member

grigasp commented Jan 6, 2025

@saskliutas, what do you think about this? IMO, our renderer should always use virtualization and the prop should not be available for consumers of our component.

I think it makes sense to allow consumer to decide whether virtualization is used or not. This TreeRenderer is one of the building blocks we provide so it might be used to build custom tree that do not require virtualization.

Sounds good. Should style prop also be changed to have effect?

@anmolshres98 anmolshres98 enabled auto-merge (squash) January 7, 2025 15:35
@saskliutas
Copy link
Member

@saskliutas, what do you think about this? IMO, our renderer should always use virtualization and the prop should not be available for consumers of our component.

I think it makes sense to allow consumer to decide whether virtualization is used or not. This TreeRenderer is one of the building blocks we provide so it might be used to build custom tree that do not require virtualization.

Sounds good. Should style prop also be changed to have effect?

I this this could be solved in a separate PR. We should probably also review if there are more props that are exposed but not propagated down to the base iTwinUI Tree component.

Co-authored-by: Saulius Skliutas <[email protected]>
@anmolshres98 anmolshres98 merged commit 3058f26 into master Jan 7, 2025
14 checks passed
@anmolshres98 anmolshres98 deleted the as/tree-virtualization-flag branch January 7, 2025 16:01
@grigasp
Copy link
Member

grigasp commented Jan 8, 2025

@saskliutas, what do you think about this? IMO, our renderer should always use virtualization and the prop should not be available for consumers of our component.

I think it makes sense to allow consumer to decide whether virtualization is used or not. This TreeRenderer is one of the building blocks we provide so it might be used to build custom tree that do not require virtualization.

Sounds good. Should style prop also be changed to have effect?

I this this could be solved in a separate PR. We should probably also review if there are more props that are exposed but not propagated down to the base iTwinUI Tree component.

Filed a followup issue: #1137

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tree widget Tree widget or its components related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tree Widget: enableVirtualization prop passed to <TreeRenderer/> not passed down to iTwinUi's <Tree/>
3 participants