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

Feature: Shadow DOM without wrapping DIV element #146

Merged
merged 5 commits into from
Dec 26, 2024

Conversation

hbroer
Copy link
Contributor

@hbroer hbroer commented Dec 24, 2024

Background

I was playing arround with NanoJSX and found in case of using it to create WebCompontents it did wrap them into a div element because Component.update() expects the parent to be Element. This adds more elements to the DOM than needed and is not expected to happen when creating a WebComponent.

Changes

I did a small change to Component.update() to get the parent via oldElements[0].parentNode instead of oldElements[0].parentElement. I then removed the shadowroot conditions in class.buildEl() and class.appendEl() in defineAsCustomElements().

Tests

The tests are fixed by removing the wrapping <div> tags. I could not run the e2e tests localy. I only checked if it still works in my local project by mounting a component and do frequent update() calls.

Breaking Changes

This PR adds a breaking change to the "Shadow DOM" feature if someone expects the wrapping div to be rendered.

@yandeu
Copy link
Member

yandeu commented Dec 24, 2024

Thanks for your PR!

I will check if your update in src/component.ts will have a big impact.

Also, it looks more like a bug fix than a breaking change to me.

@hbroer
Copy link
Contributor Author

hbroer commented Dec 25, 2024

yea was not sure if it's a bug fix or a feature. I think it potentially fixes a workaround. :-D

@yandeu
Copy link
Member

yandeu commented Dec 26, 2024

Everything work well, so I will merge it. Thanks a lot again!

@yandeu yandeu merged commit 196f49a into nanojsx:master Dec 26, 2024
10 checks passed
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