Famous / famous-angular

Bring structure to your Famo.us apps with the power of AngularJS. Famo.us/Angular integrates seamlessly with existing Angular and Famo.us apps.
https://famo.us/angular
Mozilla Public License 2.0
1.92k stars 275 forks source link

ui-router; top-level fa-surfaces do not get removed from DOM #188

Open markmarijnissen opened 10 years ago

markmarijnissen commented 10 years ago

When using angular-ui-router, old views are not removed from the DOM.

This JSFiddle demostrates the issue.

Am I missing something here, or are there any workarounds?

graunked commented 10 years ago

Looks like like the immediate child of a ui-view can't be a <fa-surface>. As a workaround, you can use <fa-modifier> or a <fa-view> as the immediate child: http://jsfiddle.net/dyx4wq36/6/.

This is a bug, though — <fa-surface> should be removed in the same way as as <fa-modifier> and <fa-view>.

bguiz commented 10 years ago

@graunked Thanks for the workaround!

zackbrown commented 10 years ago

Investigated this some more yesterday and further optimized clean-up after route changes with 4e34a22fdc1a8f8a1a64be7ef5f2c81c1e4c21c1 — this particular issue is still unresolved though. This bug is definitely unique to fa-surfaces when they're a top-level element (just updated the issue's title to clarify this.) The severity is pretty low since it's easy to wrap the route's contents in an fa-view, but still chasing this down.

bguiz commented 10 years ago

:+1: On 22 Sep 2014 03:12, "Zack Brown" notifications@github.com wrote:

Investigated this some more yesterday and further optimized clean-up after route changes with 4e34a22 https://github.com/Famous/famous-angular/commit/4e34a22fdc1a8f8a1a64be7ef5f2c81c1e4c21c1 — this particular issue is still unresolved though. This bug is definitely unique to fa-surfaces when they're a top-level element (just updated the issue's title to clarify this.) The severity is pretty low since it's easy to wrap the route's contents in an fa-view, but still chasing this down.

— Reply to this email directly or view it on GitHub https://github.com/Famous/famous-angular/issues/188#issuecomment-56305421 .

kilianc commented 9 years ago

+1

dhakan commented 9 years ago

I'm in a project where we're having almost exactly this issue, the main difference being that covering the contents of the immediate child of ui-view with an fa-modifier or fa-view still does not flush/destruct the DOM. The result is that fa-surface elements (all of them are fa-image-surfaces) are preserved with an opacity of 0.

We asked a question on SO regarding this issue, and perhaps you've discovered something new.

Link ---> http://stackoverflow.com/questions/27361723/elements-from-other-states-are-only-invisible-but-still-present-in-current-view

Thanks in advance.

HarlemCake commented 9 years ago

+1 as well. Any movement on this?

ghaiat commented 9 years ago

+1

steveblue commented 9 years ago

+1

zackbrown commented 9 years ago

In terms of triaging this, has anyone run into a situation where wrapping the fa-surface (or fa-image-surface) in an fa-view is an unacceptable fix? If so, e.g. @dhakan, it would help us to tackle this if someone could create a codepen that reproduces the issue. This template has all of the basic dependencies set up (from a quick look, ui-router should be supported in codepen, as it uses the URL of the iframe for routing)

My hunch is that the problem here lies at the Famo.us layer, where Surfaces respond to .set() differently from how Views and other RenderNodes respond to .set() (.setting to an empty RenderNode is how we handle removing components from the render tree, since vanilla Famo.us doesn't otherwise offer this feature.) Due to this difference, a pretty in-depth work-around would be required.

One solution in the F/A layer would be to wrap every fa-surface's Surface internally with a View, but this would be API-breaking for any $famous.find operation on fa-surface. Fixing this at the Famo.us layer would require some replumbing and may come with its own breaks. In either case, being able to reproduce and then isolate the issue will help a bunch.