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

tree does not render correctly without query that filters all children #732

Closed
curtw opened this issue Sep 25, 2013 · 2 comments
Closed

Comments

@curtw
Copy link

curtw commented Sep 25, 2013

All of the tests for the 'tree' column plugin use a query that filters out all non-root (i.e. child) records. If you omit such a query then the tree will not render correctly - either children will not be visually adopted by their parent until you click on the parent, or if you use 'shouldExpand' to always expand root nodes, the children will be duplicated.

Ideally you could omit this query and the child nodes would be automatically adopted by their parents and would (by virtue of the fact that some parent claims them) not appear in the root of the tree.

The above is expensive, so for performance reasons I can see the benefit of expressing to the grid what records should be at the root, but I don't think that the 'query' member is the appropriate place to do this because forcing 'query' into this role (defining root nodes) takes away your ability to filter nodes more generally and specifically takes away your ability to filter child nodes in the tree.

Maybe it would be better to have a different parameter (maybe 'rootQuery') that would specify the root nodes, and then leave 'query' for the more general filtering of all nodes. When dgrid calls the getChild() method on the store, the second argument includes the 'query' and here the store implementer could use that query to filter child nodes.

Alternately maybe you could add a new member for filtering children which would be passed into the store's getChild() method - maybe 'childQuery'. One way or another it should be possible to tell the grid what child records to filter from the tree.

@kfranqueiro
Copy link
Member

You raise some really good points. I believe this issue was actually previously raised in #145 and has sat unresolved for far too long because I never had the chance to review the pull request there.

However, I think a simpler approach than that pull request is possible. I have a commit on a branch that I want to do more testing on before I merge (particularly to make sure I didn't overlook anything dumb for paged queries), but here it is for reference: https://github.com/kfranqueiro/dgrid/commit/e345b81bffb0077825f3859f9f396fb6d5067895

This sort of meets halfway in terms of implementing the store accordingly and updating dgrid to accommodate it. The only change to dgrid's logic is to pass the original query from the grid to getChildren via the originalQuery property of the options object. The rest of the work really happens in the store:

  • The query method defaults to not performing a deep query, to avoid needing to specify an initial filter to the grid
  • The getChildren method mixes options.originalQuery into the query object it passes to query

@kfranqueiro
Copy link
Member

After additional testing this morning I've pushed e345b81 to master. I hope to push additional tests using this implementation but they rely on some other stuff I'm working on that isn't quite ready yet.

I'm going to close this for now but if you have questions feel free to ask.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants