Skip to content

change DOM Document to Element for more flexible #827

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

Merged

Conversation

andy1xx8
Copy link
Contributor

@andy1xx8 andy1xx8 commented Sep 4, 2021

This package actually uses and manipulates DOM Element only. But it requires DOM Document as its constructor ( this is unnecessary)
For now, If they have a DOM Element called element and I want to use this library, I have to create a new DOM Document. And this is quite complex and adds more boilerplate code.

In fact, it's more flexible for the users to pass a DOM element instead of DOM Document.
- If they have a DOM Document call doc , they can use doc.documentElement.
- If they have. a DOM Element called element, they can use element directly.

@tneotia
Copy link
Contributor

tneotia commented Sep 7, 2021

This is interesting but also very breaking for people using the fromDom constructor. Id be interested to hear what @erickok has to say on this one.

@andy1xx8
Copy link
Contributor Author

andy1xx8 commented Sep 8, 2021

@tneotia Yes, i do agree. I think we can add another constructor like fromElement

@erickok
Copy link
Contributor

erickok commented Sep 8, 2021

I think its a good idea to have the extra constructor which accept an Element, next to the existing 2 constructors.

@erickok
Copy link
Contributor

erickok commented Oct 21, 2021

@andy1xx8 did you plan on updating this PR to add a fromElement (instead of replacing fromDocument)?

@andy1xx8
Copy link
Contributor Author

@erickok yes i will update this in 1 - 2 days.

@andy1xx8
Copy link
Contributor Author

andy1xx8 commented Jan 8, 2022

@erickok I jave just add a new constructor fromElement instead of replacing fromDocument
Could you please help to ro review and approve this PR.
Thanks

@erickok erickok merged commit 9adc596 into Sub6Resources:master Jan 14, 2022
@erickok
Copy link
Contributor

erickok commented Jan 14, 2022

Thanks for your contribution!

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