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

Change private methods to protected #247

Closed
wants to merge 1 commit into from
Closed

Change private methods to protected #247

wants to merge 1 commit into from

Conversation

metalinspired
Copy link

Long story short, I needed to add some custom functionality for a project I'm working on but since some of the HtmlConverter methods are private I would have to needlessly duplicate these private methods.

@colinodell
Copy link
Member

Thanks for the contribution!

I generally don't like exposing private functions, but I can see how making createDOMDocument protected would be useful. But I'm less convinced about the other two methods. Would you be able to share what you're trying to do with those other methods and why making them protected would help you?

@metalinspired
Copy link
Author

metalinspired commented Feb 9, 2024

I actually solved my problem by approaching it from a completely different angle.
It is a legacy application and rich text editor it is using generates invalid nested lists.
For example: <ul><li>foo</li><ul><li>bar</li></ul></ul>
Instead of: <ul><li>foo<ul><li>bar</li></ul></li></ul>
My original intent was to fix this within DOM but, as it turns out, nested list gets appended to root element because they are, well, invalid.
This, in turn, was the reason why HtmlConverter was producing unexpected result.
I'm not sure if there is any need for this PR so close it if you wish :)

@colinodell
Copy link
Member

Ah okay, glad you were able to solve it!

@colinodell colinodell closed this Feb 9, 2024
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.

None yet

2 participants