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

[ Add ] Utility methods of ParentNode & ChildNode interfaces #294

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@TechQuery
Copy link

TechQuery commented Nov 16, 2018

Reference Issue

Fixes #288

@TechQuery TechQuery requested review from azakus and sorvell as code owners Nov 16, 2018

@TimvdLippe
Copy link
Contributor

TimvdLippe left a comment

Please add braces to all if-statements. Also, please add regression tests for these methods.

@TechQuery

This comment has been minimized.

Copy link
Author

TechQuery commented Nov 19, 2018

@TimvdLippe
I found a bug of Building scripts: only the name of prepend() is transformed, I don't know why...
image

@TimvdLippe
Copy link
Contributor

TimvdLippe left a comment

I think all methods need the following JSdoc notation:

/**
  * @this {HTMLElement}
  */

This is also going to clash with #298, but I think it is better to wait for that PR to land before going ahead with this one.

<script>
ShadyDOM = {force: true};
</script>
<!-- <script src="../shadydom.min.js"></script> -->

This comment has been minimized.

@TimvdLippe

TimvdLippe Nov 19, 2018

Contributor

Uncomment this file.

This comment has been minimized.

@TechQuery

TechQuery Nov 19, 2018

Author

If you uncomment this line, you'll meet this bug.

This comment has been minimized.

@TimvdLippe

TimvdLippe Nov 19, 2018

Contributor

Per my review comment, adding the following should fix that:

/**
  * @this {HTMLElement}
  */

This comment has been minimized.

@TechQuery

TechQuery Nov 20, 2018

Author

I have tried to add @this, but nothing changed...

Show resolved Hide resolved tests/parent-child-node.html Outdated
Show resolved Hide resolved tests/parent-child-node.html Outdated
Show resolved Hide resolved tests/parent-child-node.html Outdated
Show resolved Hide resolved src/patch-builtins.js

const fragment = document.createDocumentFragment();

for (let node of list)

This comment has been minimized.

@TimvdLippe

TimvdLippe Nov 19, 2018

Contributor

Braces here as well.

Show resolved Hide resolved package.json Outdated
Show resolved Hide resolved .editorconfig Outdated
Show resolved Hide resolved src/patch-builtins.js Outdated
Show resolved Hide resolved tests/parent-child-node.html Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment