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

Fix #24 Table not updating after tree has changed #43

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Stromwerk
Copy link
Contributor

This commit fixes the issue of the table not
updating as the tree passed to it changed.
This is done by implementing OnChanges and
rerunning initialization code after the tree
has changed

This commit fixes the issue of the table not
updating as the tree passed to it changed.
This is done by implementing OnChanges and
rerunning initialization code after the tree
has changed
@mlrv
Copy link
Owner

mlrv commented Jan 21, 2020

Thanks for this @Stromwerk! It's weird that github still seems to be waiting for Travis to complete... I'll see if refreshing a build fixes it.

The change looks great at a first glance, I'll find some time to review it more carefully later on today 👍

@mlrv
Copy link
Owner

mlrv commented Jan 24, 2020

Sorry for the delay in reviewing this @Stromwerk, the change looks good, but there's a lot of duplicated code that could be shared among ngOnInit and ngOnChange. Also, it'd be nice if we could update the tree options as well if the input changes, not just the tree itself.

I feel like it'd be enough to simply take the code that's currently run in ngOnInit and move it to ngOnChange directly, that should take care of everything.

Let me know if this makes sense to you and if you're willing to give it a go.

@Stromwerk
Copy link
Contributor Author

I completely agree with you that there is unnecessary code duplication, but I seem to recall trying to move everything in ngOnInit into ngOnChange and I got some nasty bugs which I couldn't quite track down. I would need to do some test myself to see how it behaves, but I remember also trying to refactor the duplicated code, but that also introduced some problems of it's own.

I will look into the implementation and test it to see how it could be improved.

@peterjester
Copy link

Tested this out locally, and it works really well.

One thing I did notice, and maybe I am doing something wrong, is that when data is updated, it will revert back to the default isExpanded, i.e. if a row is closed when new data is emitted, the entire row will re-open. Is there a way to set the isExpanded to default to closed either in html or programatically?

@mlrv
Copy link
Owner

mlrv commented Feb 11, 2020

Hey @peterjester , thanks for the feedback. I should have some time to properly implement this at some point in the next few days, I'll make sure to include appropriate testing for what you mentioned as well.

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.

3 participants