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

Avoid deleting the HTML node when destroying ViewWrapper instance and set visible state correctly #337

Merged
merged 3 commits into from
Jun 12, 2020

Conversation

lehnerchristian
Copy link

@lehnerchristian lehnerchristian commented Mar 27, 2020

ViewWrapper deletes its own node from the DOM

for a description, please see the linked ticket

please test this behavior in your applications if it has any side-effects.

but as far as I can see it shouldn't. the current behavior of our detail views is that the ViewWrapper instances are created once and then the detail views are only shown and hidden via a CSS class.

but node of those instances should not be deleted, because the ViewWrapper class is only instantiated when the detail view chooser is built.

as far as I can see the destroy method of the ViewWrapper class was never called in the projects I have set up locally

Edit:

another option would be to insert the HTML node again into the DOM, because we still have the reference to it in this.node

Visible state is kept when destroying the instance

by setting the visible attribute of the ViewWrapper instance back to false we avoid recrating a view when we click on the centra LineUp table. see https://github.com/datavisyn/tdp_core/blob/develop/src/base/ViewWrapper.ts#L298-L300

@lehnerchristian
Copy link
Author

lehnerchristian commented Mar 31, 2020

I've close #336 , because we agreed on destroying the ViewWrapper should of course actually delete the HTML node.

New Description:

The actual problem was that when the visible setter is called on a ViewWrapper instance the update method is invoked.

The update method checked if the instance is actually available and && this.node.getBoundingClientRect().width > 0 which not true in this case.

the size of the node depends on the active CSS class of the HTML element the detail views are in, which is set from the outside and currently app specific as it looks to me.

however if this check is present my React view does not render correctly as it is not updated anymore

@thinkh , @puehringer and @dg-datavisyn , please re-review and check with your apps

Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

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

This change does not affect Ordino, since Ordino uses its own ViewWrapper class ... (see Caleydo/ordino). Hence, I will approve this change.

@thinkh thinkh merged commit ccbb977 into develop Jun 12, 2020
@thinkh thinkh deleted the clehner/336_correct_behavior_when_view_is_destroyed branch June 12, 2020 15:28
@thinkh thinkh mentioned this pull request Jul 30, 2020
48 tasks
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.

4 participants