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

TreeView: Add virtualization support #5689

Merged
merged 22 commits into from
Oct 22, 2024
Merged

Conversation

ddjerqq
Copy link
Contributor

@ddjerqq ddjerqq commented Aug 14, 2024

Added virtualization support to the TreeView component

Closes #5649

How Has This Been Tested?

I ran the demo project and checked that the TreeView worked.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality not to work as expected)

Checklist:

  • The PR is submitted to the correct branch (master for dev, rel-1.x for maintenance).
  • My code follows the code style of this project.
  • I've added relevant tests.

@ddjerqq ddjerqq added the Type: Feature ⚙ Request or idea for a new feature. label Aug 14, 2024
@ddjerqq ddjerqq requested a review from stsrki August 14, 2024 09:29
@ddjerqq ddjerqq self-assigned this Aug 14, 2024
Copy link
Contributor

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


ddjerqq seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request

Copy link
Collaborator

@stsrki stsrki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please add release notes to the 2024-10-15-release-notes-170.razor.

ddjerqq and others added 6 commits August 14, 2024 14:45
@stsrki stsrki requested a review from David-Moreira August 18, 2024 11:12
@stsrki stsrki mentioned this pull request Aug 21, 2024
@David-Moreira
Copy link
Contributor

David-Moreira commented Aug 23, 2024

DOCS:
1. Why are all the overflow bars visible by default? I'd use a more nicer example, where only the required vertical bar is shown. Also Do you need the outside one also?

image

Example from DataGrid :
image

2. By setting Virtualize to true should we not have sane defaults where it would just work? Why are we making our users have to manually set additional settings? In my opinion it should automatically work, but users can further customize it if they want to, changing max height, etc...

3. Can we see more nesting? What will happen? Will it work ok? Will it be a bunch of vertical bars?
EDIT : 3.1 I suspect it might not work correctly not sure? Since Virtualize really likes fixed dimensions...
3.2: I'd like to see the Virtualize working also on the root items.

5. How is dynamic data handled? Imagine I had data to the treeview will it break anything? would be nice to adapt the DEMO version to work with it, since you can already do alot of interactions with the treeview there, and you can check how the Virtualize affects these.

@stsrki
Copy link
Collaborator

stsrki commented Aug 23, 2024

1. Why are all the overflow bars visible by default? I'd use a more nicer example, where only the required vertical bar is shown. Also Do you need the outside one also?

It seems this is the problem of the bigger picture. We pass all the parameters that were assigned to the root treeview elements down to the child tree nodes. I think we should not pass the down, only the necessary that are needed for the child nodes to get the data and states.

image

@David-Moreira
Copy link
Contributor

1. Why are all the overflow bars visible by default? I'd use a more nicer example, where only the required vertical bar is shown. Also Do you need the outside one also?

It seems this is the problem of the bigger picture. We pass all the parameters that were assigned to the root treeview elements down to the child tree nodes. I think we should not pass the down, only the necessary that are needed for the child nodes to get the data and states.

image

Probably not sure? Could these be acting as a king of global way of styling all nodes? So if you remove these, some TreeViews that might rely on them, might get a different UI result?

@stsrki
Copy link
Collaborator

stsrki commented Aug 23, 2024

1. Why are all the overflow bars visible by default? I'd use a more nicer example, where only the required vertical bar is shown. Also Do you need the outside one also?

It seems this is the problem of the bigger picture. We pass all the parameters that were assigned to the root treeview elements down to the child tree nodes. I think we should not pass the down, only the necessary that are needed for the child nodes to get the data and states.
image

Probably not sure? Could these be acting as a king of global way of styling all nodes? So if you remove these, some TreeViews that might rely on them, might get a different UI result?

In this case yes, they act as a semi-global styles. Which is also much different than we do in any other component. We almost never pass styles from the parent down to the child component. The right approach would be to have a callback like GetSomeParameter that we already have for node elements.

I think we should revise all styles on this and only pass the ones that are not going to break the TreeVIew component. Text styling is OK. Overflow and sizing are not OK to be passed.

@stsrki
Copy link
Collaborator

stsrki commented Aug 24, 2024

I have removed styling parameters from not being passed down and tested all examples. And it seems to be working as expected.

@stsrki stsrki requested a review from David-Moreira August 24, 2024 13:42
@David-Moreira
Copy link
Contributor

@stsrki Already asking for review?

Already gone through all the points? 1, 2,3, 3.1, 3.2, 5?

@stsrki
Copy link
Collaborator

stsrki commented Aug 24, 2024

I will do the 2. point. For others, there should not be any problems.

@David-Moreira
Copy link
Contributor

I will do the 2. point. For others, there should not be any problems.

So you're telling me you don't see any problems with this?

  1. Can we see more nesting? What will happen? Will it work ok? Will it be a bunch of vertical bars?
    EDIT : 3.1 I suspect it might not work correctly not sure? Since Virtualize really likes fixed dimensions...
    3.2: I'd like to see the Virtualize working also on the root items.

@stsrki
Copy link
Collaborator

stsrki commented Aug 24, 2024

So you're telling me you don't see any problems with this?

  1. Can we see more nesting? What will happen? Will it work ok? Will it be a bunch of vertical bars?
    EDIT : 3.1 I suspect it might not work correctly not sure? Since Virtualize really likes fixed dimensions...
    3.2: I'd like to see the Virtualize working also on the root items.

It works on all levels. Root level is the one where height and overflow is defined. I tested it on multiple levels, and it works as it should.

@David-Moreira
Copy link
Contributor

David-Moreira commented Aug 31, 2024

I still get an horizontal scroll bar which I don't think it's needed.
The vertical bar shows even though there's nothing to scroll yet, I think there's a setting to make it show only there's scrollable content. (I think it's auto, but I don't remember)
website-capture_2024-8-31

Also if I disable virtualize the TreeView does not go back to the original styling.

website-capture_2024-8-31 (1)

@David-Moreira
Copy link
Contributor

David-Moreira commented Aug 31, 2024

I am getting a huge list, can you take a look?
image

I'll commit my demo code

PS: Use the AddNode button to increase the nodes

@stsrki
Copy link
Collaborator

stsrki commented Oct 16, 2024

I am getting a huge list, can you take a look? image

I'll commit my demo code

PS: Use the AddNode button to increase the nodes

I have fixed it. The problem was that we generated TreeViewNodeState each time from the original list. So, in turn, the Key would also be new, making the duplicate between renderings. I've added a simple check for reference on the .Node itself.

@stsrki
Copy link
Collaborator

stsrki commented Oct 16, 2024

@David-Moreira please re-review.

@stsrki stsrki merged commit 9c88b10 into master Oct 22, 2024
1 of 2 checks passed
@stsrki stsrki deleted the feature-treeview-add-virtualization branch October 22, 2024 10:25
@github-actions github-actions bot locked and limited conversation to collaborators Oct 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature ⚙ Request or idea for a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TreeView: add virtualization
3 participants