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

Update transitions #65

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

Conversation

danDanV1
Copy link

@danDanV1 danDanV1 commented Jan 16, 2019

Transitions are cleaned up to remove now redundant duplication of DOM elements.

One test updated as there are no longer two elements in the DOM.

All tests pass.

All the transitions in the dummy app work just fine.... Except ....!! One unrelated error to note, after forking and doing a local install BEFORE making any changes, the sending components demo http://localhost:4200/#/docs/components was throwing an error. So I wasn't able to visually test that !!

And some changes in the commits are due to vscode/prettier formatting the code.

runtime.js:6407 Uncaught TypeError: Cannot read property 'commit' of null
    at Environment.commit (runtime.js:6407)
    at Environment.commit (environment.js:266)
    at InteractiveRenderer._renderRootsTransaction (renderer.js:359)
    at InteractiveRenderer._revalidate (renderer.js:393)
    at invoke (backburner.js:205)
    at Queue.flush (backburner.js:125)
    at DeferredActionQueues.flush (backburner.js:278)
    at Backburner.end (backburner.js:410)
    at Backburner._run (backburner.js:760)
    at Backburner._join (backburner.js:736)
commit @ runtime.js:6407
commit @ environment.js:266
_renderRootsTransaction @ renderer.js:359
_revalidate @ renderer.js:393
invoke @ backburner.js:205
flush @ backburner.js:125
flush @ backburner.js:278
end @ backburner.js:410
_run @ backburner.js:760
_join @ backburner.js:736
join @ backburner.js:477
run.join @ ember-metal.js:4366
handler @ action.js:102
(anonymous) @ event_dispatcher.js:234
dispatch @ jquery.js:5183
elemData.handle @ jquery.js:4991
liquid-destination.js:63 Uncaught TypeError: Cannot read property 'find' of undefined
    at Class.removeWormhole (liquid-destination.js:63)
    at Class.removeWormhole (liquid-wormhole.js:35)
    at Class.willDestroyElement (liquid-wormhole.js:53)
    at Class.superWrapper [as willDestroyElement] (ember-utils.js:420)
    at Class.trigger (core_view.js:62)
    at Class.superWrapper [as trigger] (ember-utils.js:420)
    at ComponentStateBucket.destroy (curly-component-state-bucket.js:34)
    at SimpleBlockTracker.destroy (runtime.js:2171)
    at UpdatableBlockTracker.destroy (runtime.js:2171)
    at SimpleBlockTracker.destroy (runtime.js:2171)

Fixes issue:
#64

@pzuraq
Copy link
Owner

pzuraq commented Jan 16, 2019

The behavior of some of the tests has changed, looking at them locally, but it looks like the behavior had already changed on master as well. Specifically, the nested wormholes test changes before and after this change.

Will need to dive back in to figure out what I was thinking at the time, and why the cloning existed in the first place. It was there for a reason, that's all I can really say 😕

@danDanV1
Copy link
Author

danDanV1 commented Jan 16, 2019

Sorry, the only test I modified was components are visible during the transition. The rest should just be code formatting.

Give it a try with my changes to transitions/wormhole.js. I feel animating the clone isn't ideal, because if the component is changing during animation (say in-viewport detection) it's element is hidden and the changes aren't displayed. The static DOM copy is what is animating. If you need two elements in there, perhaps you can animate the live copy? and use the static one as the placeholder, which I assume might be the reason it's there?

Let me know if there is anything else I can lend a hand with.

@pzuraq
Copy link
Owner

pzuraq commented Jan 16, 2019

What I meant was when I ran the test suite locally before and after this change, there were some visual differences even though the tests passed. The tests were not that comprehensive when I wrote them, I don’t really trust them without an actual visual audit.

However, that audit also appears to have failed prior to this change, as in something else caused a change before this PR. So, they’re broken, and this PR breaks them further.

I 100% agree that the cloning is bad, I have wanted to rewrite it for years now, but I need to do my due diligence here and make sure we aren’t breaking people’s apps. I’m not sure when I’ll have time to dive back in and figure out why I added it in the first place, or what caused the inital breakage, but I want to be thorough here. If you’d like to do that investigation, happy to work with you through that process (and definitely add more tests).

@danDanV1
Copy link
Author

danDanV1 commented Jan 17, 2019

I took a stab at doing a bit of clean up in my update-dependancies branch https://github.com/edeis53/liquid-wormhole/tree/update-dependencies

I upgraded to Ember 3.7, changed the markdown compiler to get rid of babel deprecations, and removed jquery. Inserting of the wormhole elements went well on transition in, but there was a bit of a state issue on the component with switching to vanilla js for inserting the component on the transition out, which wasn't an issue with jquery. I'm still not 100% on how you have those parts designed and why it works the way it does. Even though I was using vanilla javascript methods that were suppose to be the same as the jquery equivalents, there were some querks on how it handles the dom elements.

For my current project, my fork (pre-jquery removal attempt #0931445) works awesome and I don't see any issues yet. I'm going to go with that for now. I'll give you a chance to revisit a bit, as will I as I get more experience integrating liquid-wormhole.

@RobbieTheWagner
Copy link
Collaborator

@danDanV1 Hello! I realize this is very old now, but I just got everything updated to modern Ember and with embroider support. Would you be interested in rebasing and continuing this work?

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