Skip to content

Conversation

@rlivsey
Copy link
Collaborator

@rlivsey rlivsey commented Mar 15, 2016

ember-mobiledoc-editor uses ember-wormhole to insert components into
atom placeholders. This immediately causes a mutation and then
the editor update is triggered when nothing has changed.

Atoms are their own world and so any changes in them should be safe
to ignore.

@rlivsey
Copy link
Collaborator Author

rlivsey commented Mar 15, 2016

If this is good, we might need to do the same for cards as they are inserted in the same way using wormholes in ember-mobiledoc-editor?

@rlivsey rlivsey force-pushed the ignore-atom-mutations branch from e12986c to 63bd4bb Compare March 15, 2016 16:06
ember-mobiledoc-editor uses ember-wormhole to insert components into 
atom placeholders. This immediately causes a mutation and then
the editor update is triggered when nothing has changed.

Atoms are their own world and so any changes in them should be safe
to ignore.
@rlivsey rlivsey force-pushed the ignore-atom-mutations branch from 63bd4bb to 192d626 Compare March 15, 2016 16:15
@rlivsey
Copy link
Collaborator Author

rlivsey commented Mar 15, 2016

Failure is saucelabs

@bantic
Copy link
Collaborator

bantic commented Mar 16, 2016

Thanks for this, makes total sense to me. I am wondering if it would be easier to delegate the responsibility to the render node the way we do for sections with renderNode.reparsesMutationOfChildNode.
I'll take a look at those sauce failures. I restarted the CI build for this yesterday but it seems to have failed in the same way again.

@rlivsey
Copy link
Collaborator Author

rlivsey commented Mar 16, 2016

Makes sense for this to be a renderNode concern, I'll take a look at that


function closest(el, selector, contentOnly=false) {
if (!isElementNode(el)) {
el = el.parentElement;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be el = el.parentNode in order for IE to pass. IE doesn't define parentElement on Node, only on Element (and a textNode is a Node but not an Element). (Thanks a lot, IE.)

@bantic
Copy link
Collaborator

bantic commented Mar 16, 2016

@rlivsey I added a few comments. This looks good to me. I will do a mini spike to try finding a render node (instead of a section render node) for the mutations to check the feasibility of simply delegating the shouldReparse logic to the render node itself, that feels a bit cleaner and also potentially more future-safe.

@rlivsey
Copy link
Collaborator Author

rlivsey commented Mar 16, 2016

@bantic thanks, I'll make those tweaks later tonight

@bantic
Copy link
Collaborator

bantic commented Mar 16, 2016

@rlivsey When you have a chance could you test this branch out? Moving reparsing logic onto the render nodes seems to work reasonably well:
master...ignore-atom-mutations-via-render-nodes

@bantic
Copy link
Collaborator

bantic commented Apr 7, 2016

@rlivsey Ping. Did you have a chance to check out the spike on the branch I mentioned before? If that works I'd be happy to fold it in and finish up work here. Would love to get this merged.

@bantic
Copy link
Collaborator

bantic commented Apr 19, 2016

@rlivsey Thanks for this PR! I rebased and added a few additions (delegating reparsing to render nodes, mostly) in #362. thanks!

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.

2 participants