grapoza / vue-tree

Tree components for Vue 3
MIT License
95 stars 12 forks source link

Support for filtering #296

Closed luckyrat closed 1 year ago

luckyrat commented 1 year ago

Is your feature request related to a problem? Please describe. I need to be able to filter the rows that are displayed in the tree, showing only those items whose labels include a search term or have children that match.

Describe the solution you'd like I'd like some way to do this. According to #4 it is up to me to modify the initialModel array when the list of items to be rendered has changed. However, it appears that the component adds the treeNodeSpec metadata only once so when I supply a new tree structure with the necessary filtering already applied, the component crashes because it can't find the required metadata.

It might be easiest if this were implemented with some sort of "hidden" flag on the treeNodeSpec (although unlike the suggestion in #4, I'm not sure that there is a need for the callback - the parent can just modify the objects in the same array that is passed into initialModel, assuming that the treeview component is set up to react to those changes automatically when next rendering).

Describe alternatives you've considered Before every filtering operation, walk the currently visible tree (the array supplied to the initialModel prop) and clone every object into the corresponding location in a shadow tree array stored in my parent component. Then directly manipulate initialModel's array by either removing objects or adding new ones back into the tree from the shadow tree, thus (hopefully!) re-creating the exact state the item was in when it was removed from the tree. I'm not sure if this will break the reactivity of the restored tree though.

I'm not certain it should really be up to a parent component to have to worry about storing and recreating every property in this component's treeNodeSpec but as long as there isn't any other internal metadata in the component which could get out of sync, this idea might at least be achievable without changes being required to this component.

Any thoughts on how best to approach this from both an immediate quick-solution perspective and a longer-term potential API change / new feature development perspective?

Thanks for your work so far on this project, it is very close to being the best Vue treeview component available, at least for my use cases!

grapoza commented 1 year ago

Hey Chris, I absolutely agree the parent component should never need to manage the lifecycle of the treeNodeSpec. In the short term, the shadow tree would probably be a valid workaround, but is really heavy for something as common as filtering. That's a huge ask from a component, and shouldn't be needed.

Just to make sure we're on the same page, when a new node is added to the initialModel prop it should get the treeNodeSpec data added (with default values and any supplied modelDefaults applied). It sounds like you're replacing the whole tree structure with a filtered tree, which as you noted breaks the tree's internals. When taking a look at that stackblitz with the Vue Devtools it seems like the nodes themselves actually do get all the expected data in the treeNodeSpec, but the tree's own initialModel never gets updated with those changes somehow. I wonder if this broke when moving to Vue 3 as part of 4.x or during the composition refactoring as part of 5.x?

I think there are two possible paths forward long-term:

  1. Figure out what's going on with the initialModel issue here, and mitigate it (with a fix or a reworking of the internals somehow)
  2. Use something like a new treeNodeSpec.state.hidden attribute to filter the top-level and child nodes. If the current internals take this into account when computing child lists then it can probably be done without having to think about keyboard navigation etc.

I'll see if I can put some more thought into this this weekend when I'm fresher. I'll try to keep this issue updated with any findings and any decision on a design. Thanks for submitting this one.

luckyrat commented 1 year ago

I couldn't get the shadow tree idea to work. I think that even when adding the same object that was previously extracted from the initialModel tree, that has somehow lost an internal association (maybe a Vue Proxy or something else you track within the component - I'm not familiar enough with either to be sure).

Thanks for the example links. I've not used that site before but think I have managed to create a fork of the 2nd example which you can access here: https://stackblitz.com/edit/vitejs-vite-mughet?file=src/App.vue

I've tweaked your replacement function to more closely match the algorithm I've used to perform the replacement - I don't see any difference in behaviour from the example you wrote but thought it might be useful to see it anyway (and I've not hooked up Vue dev tools to investigate any of this yet so perhaps some differences would become apparent there).

I also added a 2nd replacement function which lets us explore what happens when a new node is submitted with a full set of treeNodeSpec properties, since that is similar to what would be needed to make the shadow tree idea work.

It's interesting that popping and pushing in your first example works but a push in replaceTreeData() in my forked 2nd example does not.

I am replacing the entire tree, although as you can see in my forked example, trying to do so in a way that doesn't affect the object (Array) reference that was passed to the initialModel prop. I'm not sure how else I could approach it since any number of individual child items, or even all children from a particular node could need to be hidden or re-added each time the filter gets re-applied.

As long as (2) can correctly react to a change to that hidden attribute when applied from a parent component (and the rest of internals can take it into account in the way you already mention) it sounds like a decent approach to me - I could just walk the same tree that I supply to initialModel and update that state whenever I need to hide or show a node.

I guess that (1) might result in some mitigation that has knock-on benefits to other interactions with the component beyond just the need for filtering out nodes. Perhaps when reviewing the internals to make (2) work, an explanation of the behaviour in (1) will magically appear, or vice-versa.

grapoza commented 1 year ago

Thanks for all the great feedback and investigation. I started a proof-of-concept for filtering yesterday and am working on it again today. I'm hoping to narrow it down to the point where you as a user don't have to care about setting things in the treeNodeSpec at all. My current thought (idea number three at this point, I think?) is to have a filter prop on the tree itself, which you can set and update with a filter function that works on a node and returns a boolean (true = show, false = hide). The tree supplies that to the nodes with provide/inject, and nodes use the filter function and the child nodes' state to compute whether they're hidden.

This would let you provide the filtering as :filter-prop-name="myFilterMethod" and even have a computed filter method that changes when your filter string changes (I'm assuming you're binding filtering to a text field or something, and this may be total overkill):

const myFilterMethod = computed(() => (node) => node.name === someFilterRef.value)

I'm not even sure that would work; I'm spitballing at this point. More likely, you'd probably update the filter method by watching your filter value, debouncing as needed, and then setting the filter method when the user pauses typing.

luckyrat commented 1 year ago

I've not created a function output from a computed value before but assuming that works reliably then this sounds like a powerful/flexible solution to me. You're right that I have a text field bound to the filtering term (which in practice would be more like a search for the presence of that substring than an equality check but the functional approach means that's not a detail you'd need to be concerned about).

I agree that the parent component should be responsible for debouncing, unless for some reason you have to handle it internally. I think it's better handled by the parent component but it's probably not a serious problem if it can't be done that way.

Thanks for the ongoing investigation. Let me know if you come up with any proof of concept code you think it would be worth me trying out.

I can't promise that this will be a simple code review but in case you are interested, the Vuetify team have previously supported a filter prop for Vue 2 and are changing it for Vue 3 ("filter prop removed. use props from filter composable", which doesn't mean much to me so I'm glad I'm not going to be updating to that if they ever finish the changes) - https://github.com/vuetifyjs/vuetify/pull/14715 in case you want to dig into their source code for fun one day! They used to offer a simple string prop which when updated would cause the tree to filter based on a substring search of the label. So nowhere near as generally useful as your idea number 3 but it did used to be good enough for my current use case.

grapoza commented 1 year ago

Alright, I've created a quick poc for filtering using the inject approach I described above (didn't touch the whole computed-as-a-param-to-the-treeview thing yet). If you grab the branch for the poc you can run the storybook task and try it out. The "Filtering" Storybook story has a text field and a button to apply the filtering, which will case-insensitively match the label text and display any matching nodes and their parents.

I haven't added support for recognizing filteredness to things like expansion/selection/traversal, but now that it's proven that I can filiter I don't think there's much stopping me from getting that stuff working.

There's some leftover stuff from previous attempts that I'll flesh out and keep around or split into a new story, like being able to do postorder depth-first traversals. You can ignore that if you take a look.

Thanks for the Veutify link, they do really cool stuff with the way they organize their composables and generalize the functionality. In a final solution the filtering for this component will hopefully get separated in a similar way, as I'll want to reuse what I can in the TreeGrid implementation I'm (slowly) working on.

grapoza commented 1 year ago

Alright, so I've basically finished the branch to implement filtering with the exception of most unit tests and looking into corner cases (data changes that affect filtering reacting appropriately in all cases?) I'll get working on those, but if you want to take a look hopefully it'll meet your needs.

luckyrat commented 1 year ago

Sorry for not getting a chance to look at the branch until now. I've built the branch locally and tried to integrate it to a branch in my project (https://github.com/kee-org/browser-addon/tree/feature/save-group-filtering). Unfortunately it's not working yet, I'm not sure why and have run out of time for today (and possibly all week).

I've checked that my filterMethod is being called and confirmed it is returning true/false as it should but the tree itself never changes (well actually some CSS class might change judging by a slight padding shift I see when the filterMethod is not null but I suspect that is irrelevant).

I've skimmed through the code changes in your branch and although my knowledge of the Vue composition API is pretty weak, it looked like a decent approach and nothing obviously wrong jumped out at me.

I'm not confident the problem lies with what you've implemented, especially since the sample in the Storybook works just fine. Maybe there are edge cases relating to enabling the selection feature, or using custom property names, or maybe I'm just missing something about how I'm configuring my component with the Options API.

I'll try to take a closer look soon but since you're planning to work on some extended edge cases and unit testing anyway, you might end up fixing whatever my problem is in the mean time.