-
Notifications
You must be signed in to change notification settings - Fork 83
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: Add doctype when parsing from string #301
Conversation
const { parser } = getTestParser() | ||
const doc = parser.parseFromString( | ||
'<!DOCTYPE html><body></body>', | ||
MIME_TYPE.HTML |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a random question if we should consider exporting these XML & HTML MIME types someday?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it makes sense to publicly export the mime types and the name spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a couple minor, optional nits
We should probably add some more blank lines to the code base ... someday.
this.doc.doctype = dt; | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearly outside the scope of the proposed changes but we should probably add a blank line here someday.
Co-authored-by: Chris Brody <chris.brody+brodybits@gmail.com>
Co-authored-by: Chris Brody <chris.brody+brodybits@gmail.com>
I merged from mobile device before the checks were done, one of the additional lines that was suggested was not OK for the linter. 64647d1 fixed that. |
fixes #277
It's not the nicest solution, since the property should actually be
readonly
, but I think it's a good fix in the current situation.