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

Do not require a component to wrap MediaQuery children #30

Merged
merged 3 commits into from
Nov 9, 2015

Conversation

jdlehman
Copy link
Contributor

@jdlehman jdlehman commented Nov 5, 2015

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:

<MediaQuery query="(min-width: 700px)">
  <MyComponent />
</MediaQuery>

will render

  <MyComponent />

instead of

  <div>
    <MyComponent />
  </div>

but

<MediaQuery query="(min-width: 700px)">
  <MyComponent />
  <MyComponent2 />
</MediaQuery>

still renders

 <div>
    <MyComponent />
    <MyComponent2 />
  </div>

- 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.
@yocontra
Copy link
Owner

yocontra commented Nov 5, 2015

Needs documentation and then LGTM

@jdlehman
Copy link
Contributor Author

jdlehman commented Nov 5, 2015

Awesome, wanted to get thoughts first. I'll add docs now.

@jdlehman
Copy link
Contributor Author

jdlehman commented Nov 6, 2015

Let me know if I need to make any changes to the docs I added.

@yocontra
Copy link
Owner

yocontra commented Nov 8, 2015

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>

@jdlehman
Copy link
Contributor Author

jdlehman commented Nov 8, 2015

Definitely a valid concern. We could also pass additional properties directly to the element.

Perhaps something like (the else if is the added part):

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;
}

@yocontra
Copy link
Owner

yocontra commented Nov 9, 2015

@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

@jdlehman
Copy link
Contributor Author

jdlehman commented Nov 9, 2015

@contra cloneWithProps is deprecated in favor of cloneElement. See here.

@yocontra
Copy link
Owner

yocontra commented Nov 9, 2015

LGTM

yocontra pushed a commit that referenced this pull request Nov 9, 2015
Do not require a component to wrap MediaQuery children
@yocontra yocontra merged commit e1ac81c into yocontra:master Nov 9, 2015
@@ -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) {
Copy link

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).

Copy link
Owner

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

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

3 participants