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

fix(html-template-element): fix template.content.cloneNode #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

StrahilKazlachev
Copy link
Contributor

@StrahilKazlachev StrahilKazlachev commented Oct 13, 2017

where native support is missing fix aurelia/templating#569

@jdanyow could you review?
I'm not sure how good idea is to reassign .cloneNode for a second time in lazySafeCloneNode.

@EisenbergEffect
Copy link
Contributor

@jdanyow Are you able to review this?

@jdanyow
Copy link
Contributor

jdanyow commented Dec 10, 2017

@StrahilKazlachev sorry for the delay- this is looking good, I agree with you, no need to use the lazy pattern here.

Would you be willing to refactor the lazy stuff out and add a test?

If you npm install karma-ie-launcher as a dev dependency, karma start --browsers IE will run the test suite against IE. This would be a great repo to do IE testing in... we do the same in binding.

@StrahilKazlachev
Copy link
Contributor Author

@jdanyow Will refactor and ping you when ready.

@StrahilKazlachev StrahilKazlachev force-pushed the fix/template-content/clone-node branch from 964fde7 to fd5331f Compare January 1, 2018 11:27
@StrahilKazlachev
Copy link
Contributor Author

@jdanyow So it took me a lot more time to come back to this 😭
Finding and caching child templates is deferred till first call to cloneNode(true) - we don't want to miss any child templates added after the parent was created - the second test for ensureHTMLTemplateElement should cover this scenario.

@EisenbergEffect
Copy link
Contributor

@jdanyow What do you think? Good to merge?

@EisenbergEffect
Copy link
Contributor

Ping @jdanyow for review.

@EisenbergEffect
Copy link
Contributor

@jdanyow Could you take another look at this?

@EisenbergEffect
Copy link
Contributor

@StrahilKazlachev Do you feel pretty good about this?

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.

Internet Explorer: ViewFactory.create does not clone child template content
3 participants