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

Allow component re-use #94

Closed
wants to merge 1 commit into from
Closed

Allow component re-use #94

wants to merge 1 commit into from

Conversation

pdf
Copy link
Contributor

@pdf pdf commented Mar 14, 2017

Partial fix for #78, only allows re-use of *vecty.Component, not
*vecty.HTML.

@pdf pdf mentioned this pull request Mar 22, 2017
@slimsag
Copy link
Member

slimsag commented Mar 28, 2017

Sorry I haven't had a chance to give this my full attention yet. I hope to investigate this on Sat/Sun.

Overall it looks good, but I'd like to make sure there aren't any regressions, which is tough to do. I'd like to land some unit tests for this code (I've been working on those, but haven't pushed anything yet)

@pdf
Copy link
Contributor Author

pdf commented May 7, 2017

I've pushed alternative logic for handling this, rather than jumping out of the standard loop into Rerender, it just adds conditions to the panic.

dom.go Outdated
@@ -207,7 +207,7 @@ func (h *HTML) Restore(old ComponentOrHTML) {
}
}

if prev, ok := old.(*HTML); ok && prev != nil {
if prev, ok := old.(*HTML); ok && prev != nil && prev.Node != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect this may fix #92 as well

Partial fix for hexops#78, only allows re-use of *vecty.Component, not
*vecty.HTML
@pdf
Copy link
Contributor Author

pdf commented May 7, 2017

I've had to revert to the original implementation, as the alternative was not reliably re-rending some children.

This was referenced May 9, 2017
@pdf
Copy link
Contributor Author

pdf commented May 10, 2017

Closing in favour of #107 meta-pull.

@pdf pdf closed this May 10, 2017
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.

None yet

2 participants