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

Use preact Fragment to render multiple children #37

Merged
merged 2 commits into from
Apr 22, 2020

Conversation

hpneo
Copy link
Contributor

@hpneo hpneo commented Mar 8, 2020

When wrap = true, preact-markup only renders the first child. Using Fragments, preact-markup can render all the children without wrapping them with a <div />.

When `wrap = true`, `preact-markup` only renders the first child. Using Fragments, `preact-markup` can render all the children without wrapping them with a `<div />`.
@yunyu
Copy link

yunyu commented Mar 26, 2020

@developit Would you mind merging this?

@hpneo
Copy link
Contributor Author

hpneo commented Apr 22, 2020

@developit @marvinhagemeister what do you think of this PR?

src/index.js Outdated
@@ -49,7 +49,7 @@ export default class Markup extends Component {
}
}

if (wrap===false) return vdom && vdom[0] || null;
if (wrap===false) return vdom && h(Fragment, {}, vdom) || null;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need for Fragment here - this line can just return the array:

if (wrap===false) return vdom || null;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@developit
Copy link
Owner

Good idea! I left a suggestion to simplify things.

I'm actually thinking we can just drop the wrap parameter now, since this component can just always return the array of elements.

@hpneo
Copy link
Contributor Author

hpneo commented Apr 22, 2020

Thanks @developit! I applied your suggestion.

For backwards compatibility, maybe we can keep the wrap parameter?

@developit
Copy link
Owner

@hpneo yeah that works for me!

@developit developit merged commit 1899ca0 into developit:master Apr 22, 2020
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