-
-
Notifications
You must be signed in to change notification settings - Fork 296
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
Do not require a component to wrap MediaQuery children #30
Conversation
- If a props.component was not explicitly defined and there is only 1 child inside the MediaQuery, just render the child instead of wrapping it in a div.
Needs documentation and then LGTM |
Awesome, wanted to get thoughts first. I'll add docs now. |
0fcee5a
to
56ecbb2
Compare
Let me know if I need to make any changes to the docs I added. |
What do you think about also rendering the component if any additional properties were defined on the MediaQuery element? I could see this being confusing: <MediaQuery query="(min-width: 700px)" onClick={whatever}>
<MyComponent />
</MediaQuery> |
Definitely a valid concern. We could also pass additional properties directly to the element. Perhaps something like (the if (this.props.component || this.props.children.length > 1) {
return React.createElement(
this.props.component || 'div',
props,
this.props.children
);
} else if (props) {
return React.cloneElement(this.props.children, props);
} else {
return this.props.children;
} |
@jdlehman Wouldn't cloneWithProps solve this problem? When only one element is provided we can do cloneWithProps and that makes sure everything is passed through to the child |
LGTM |
Do not require a component to wrap MediaQuery children
@@ -87,7 +86,20 @@ var mq = React.createClass({ | |||
return null; | |||
} | |||
var props = omit(this.props, excludedPropKeys); | |||
return React.createElement(this.props.component, props, this.props.children); | |||
if (this.props.component || this.props.children.length > 1) { |
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.
FYI children
is an opaque data structure and its internal representation can change. It’s better to use React.Children.count(this.props.children)
.
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.
@gaearon Thanks for the heads up
If a
props.component
was not explicitly defined and there is only 1 child inside the MediaQuery, just render the child instead of wrapping it in a div automatically.This is a breaking change as it requires
props.component
to be explicitly defined if it only has one child, but it also provides more flexibility and makes it easier to limit unneeded "wrapper" DOM elements.Example
The following:
will render
instead of
but
still renders