-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Render void elements as self-closing tags #2064
Render void elements as self-closing tags #2064
Conversation
🦋 Changeset detectedLatest commit: 437d6c7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✔️ Deploy Preview for astro-docs-2 ready! 🔨 Explore the source changes: 437d6c7 🔍 Inspect the deploy log: https://app.netlify.com/sites/astro-docs-2/deploys/61a7dc45d6807700079ddf79 😎 Browse the preview: https://deploy-preview-2064--astro-docs-2.netlify.app |
✔️ Deploy Preview for astro-www ready! 🔨 Explore the source changes: 437d6c7 🔍 Inspect the deploy log: https://app.netlify.com/sites/astro-www/deploys/61a7dc459dfbec0008593276 😎 Browse the preview: https://deploy-preview-2064--astro-www.netlify.app |
Can this just be |
@@ -223,7 +225,7 @@ If you're still stuck, please open an issue on GitHub or join us at https://astr | |||
// This is a custom element without a renderer. Because of that, render it | |||
// as a string and the user is responsible for adding a script tag for the component definition. | |||
if (!html && typeof Component === 'string') { | |||
html = await renderAstroComponent(await render`<${Component}${spreadAttributes(props)}>${children}</${Component}>`); | |||
html = await renderAstroComponent(await render`<${Component}${spreadAttributes(props)}${voidElementNames.test(Component) ? `/>` : `>${children}</${Component}>`}`); |
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.
Can this just be
>
and not/>
?
Yes, it could. My preference is to be explicit.
Could we add a few tests for this, maybe in |
Some tests of the capability have been added, @drwpow. |
expect($('body > :nth-child(1)').prop('outerHTML')).to.equal('<input>'); | ||
|
||
// <Input type="password" /> | ||
expect($('body > :nth-child(2)').prop('outerHTML')).to.equal('<input type="password">'); |
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.
Awesome tests!
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.
Great tests, thank you! And good fix
@@ -223,7 +225,9 @@ If you're still stuck, please open an issue on GitHub or join us at https://astr | |||
// This is a custom element without a renderer. Because of that, render it | |||
// as a string and the user is responsible for adding a script tag for the component definition. | |||
if (!html && typeof Component === 'string') { | |||
html = await renderAstroComponent(await render`<${Component}${spreadAttributes(props)}>${children}</${Component}>`); | |||
html = await renderAstroComponent( | |||
await render`<${Component}${spreadAttributes(props)}${(children == null || children == '') && voidElementNames.test(Component) ? `/>` : `>${children}</${Component}>`}` |
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.
Eh, ==
? Yea, it’s that or children === undefined || children === null || String(children) === ''
. This is knowingly loose. And the linter doesn’t mind. 😅
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.
I think this is good enough. I can’t think of a problem with ==
here
* Render void elements as self-closing tags * changeset * nit: only check void element names if there is no child content * nit: only check void element names if there is no child content * add tests
* Render void elements as self-closing tags * changeset * nit: only check void element names if there is no child content * nit: only check void element names if there is no child content * add tests
Changes
Testing
no tests added
Docs
bug fix only