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

Feature Child Row Tmpl (Sub-Tables) #403

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

ClarenceL
Copy link
Contributor

@ClarenceL ClarenceL commented Jun 19, 2016

Responding to comments raised in:
#296

Would you be able to use the tmpl option and write an external template with the expand button and helpers for the subtable?

This is an updated feature set to provide fully customizable expanded rows, and a fallback to reactive-table style child tables, it provides a simple setting to enable the expand icon on any column, as well as helpers for the subtable if desired.

With this you can easily nest entire "reactive-tables" within each other, this is pretty handy and allows me to achieve many layers of tables

Please see separate readme file:
CHILD_TABLE_README.md

@U54
Copy link

U54 commented Jun 21, 2016

Edit:

When creating a custom expand button I would receive the following errors in my console when clicking the button:

Uncaught TypeError: this.expandChildren is not a function
Uncaught TypeError: this.collapseChildren is not a function

I was able to fix this by changing this.collapseChildren(); and this.expandChildren(); as shown in the CHILD_TABLE_README.md to this._collapseChildren(); and this._expandChildren(); (note the '_').

Once again thanks for making this, it's really helping me out so far and I'm sure it will help a lot of other people too! Hope to see it integrated as an official feature or become some kind of addon package if possible.

@ClarenceL
Copy link
Contributor Author

Ah sorry yes, I added the underscore to avoid conflicts but didn't update the documentation, I think there's a better way to do this but for now it'll do.

And yes I just built a triple nested reactive-table so it was sorely needed on my end.

@aslagle
Copy link
Owner

aslagle commented Jun 26, 2016

Hi @ClarenceL, thanks a lot for working on this!

This is a very useful feature and I like the way it's going, but I do have some more comments.

First, I'd like a demo app in the examples directory that someone can run to see what an expanded child looks like - either a new app or incorporate this functionality into one of the existing apps.

Second, I think the options could be simplified quite a bit. Since this is an advanced feature, I think most people who use it will want to be able to customize it. It's important to keep the options flexible but not as important to make it easy to get set up. It's ok if people need to write more code to use it, as long as they're able to configure it to work however they'd like.

So on that note, do you need to have the children.fields option for a nested reactive table? Couldn't someone use tmpl for that and put a reactive table in their template, if there was good documentation of how to do it? They'd have more control over other table options and it would be easier to maintain this code if there was only one option.

Similarly, I'm not sure expandButton needs to be an option. The helpers could be added to the regular tmpl option, and people could put their own expand button in their template. Then they'd have more control over how the expandButton appears with respect to the content, and could show or hide it depending on what the content is.

Finally, you could consider making the visibleChildren ReactiveVar an option that can be passed into the table settings. I'm not sure if this makes sense or not, but it would give people more flexibility than using the built-in helpers - for example, if you wanted to have a button outside the table that collapses or expands all rows, or if you wanted to make expanding one row automatically collapse all the others, you could modify the reactive var directly.

Thanks again for doing this, let me know what you think!

{{> ../../children/tmpl}}
{{/with}}
{{else}}
<span class="error">if children field exists, you must have defined either tmpl or fields</span>
Copy link
Owner

Choose a reason for hiding this comment

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

This is kind of a nitpick, but I don't think we should have any errors that could accidentally get displayed to an end user if there's a bug in someone's code. setup in reactive-table.js can check all the children and log an error to the console if there are problems.

@ClarenceL
Copy link
Contributor Author

Thanks for the comments, I will definitely look at them shortly, briefly skimming you're totally right about improving the ReactiveVar for the visibleChildren, I found in my testing that if the data changed reactively then this ReactiveVar was re-created, therefore any expanded rows would close, this is quite jarring.

I think the only solution is to allow the developer to pass in a session key they'd like to use to track these, and as a Session var it would persist after a data reactive update.

@aslagle
Copy link
Owner

aslagle commented Jul 2, 2016

For the other arguments, I've used ReactiveVar so the developer can set up a reactive function that persists the changes - to a Session variable, local storage, a database, or wherever else they want.

@aslagle
Copy link
Owner

aslagle commented Jul 2, 2016

You don't need to use a Session key, you just need to let the developer have access to the ReactiveVar.

ayselafsar and others added 3 commits December 1, 2016 15:13
Add expandIcon and collapseIcon properties to have dynamic icons, Exp…
@juliomac
Copy link

Hi @ClarenceL and @aslagle , this is quite an awesome package! Thanks for that!

I find very interesting the Child Table feature. Is the merge resolved? It would be a awesome addition to the package!

@ClarenceL
Copy link
Contributor Author

I've unfortunately stopped working with Meteor so I don't know when I'll have time to review the comments and merge this properly.

Check template has visible children
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

Successfully merging this pull request may close these issues.

6 participants