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

Hidden does not render multiple children regardless of breakpoints #8072

Closed
Izhaki opened this issue Sep 6, 2017 · 4 comments
Closed

Hidden does not render multiple children regardless of breakpoints #8072

Izhaki opened this issue Sep 6, 2017 · 4 comments
Assignees
Labels
bug 🐛 Something doesn't work

Comments

@Izhaki
Copy link
Contributor

Izhaki commented Sep 6, 2017

When Hidden is provided with more than one child (at least in the css implementation), it does not throw an error and simply hides the content altogether.

<Hidden xsDown implementation="css">
  <NavLink href="/">Home</NavLink>
  <NavLink href="/audio">Audio</NavLink>
</Hidden>
  • Material-UI: 1.0.0-beta.8

This is probably due to this check:

  if (!React.isValidElement(children)) {
    return null;
  }

which I assume returns false being given more than one element.

Before looking deeper into this, I'm not sure if this is actually the desired behaviour:

  • why not allow more than one child?
  • why is this check is there to begin with?

May submit a PR.

@Izhaki Izhaki changed the title Hidden simply hides if children is not a single element Hidden always hides multiple children Sep 6, 2017
@rosskevin
Copy link
Member

rosskevin commented Sep 6, 2017

I expect Hidden and it's implementations HiddenJs and HiddenCss should allow multiple children, but it appears that it is typed for a single child.

I also expect that it hides all children.

@oliviertassinari is there a reason why a flow type restriction exists to a single child? I would expect the type to be children?: $ReadOnlyArray<ChildrenArray<Node>>,, meaning undefined to many of anything.

@rosskevin rosskevin added the v1 label Sep 6, 2017
@oliviertassinari
Copy link
Member

@rosskevin No reason I can remember. I was fighting with flow to get a green build.

@rosskevin
Copy link
Member

rosskevin commented Sep 7, 2017

why not allow more than one child?

@Izhaki I just submitted a PR to fix the flow type.

(isValidElemen) why is this check is there to begin with?

I assume it is there to prevent React.cloneElement from failing.

Here is the implementation:

/**
 * Verifies the object is a ReactElement.
 * See https://facebook.github.io/react/docs/top-level-api.html#react.isvalidelement
 * @param {?object} object
 * @return {boolean} True if `object` is a valid component.
 * @final
 */
ReactElement.isValidElement = function (object) {
  return typeof object === 'object' && object !== null && object.$$typeof === REACT_ELEMENT_TYPE;
};

Now, the title of your issue is Hidden always hides multiple children, which is what I expect.

So, with all of that said (and children type fixed), I don't think I understand the issue you have. Can you explain?

@Izhaki Izhaki changed the title Hidden always hides multiple children Hidden does not render multiple children regardless of breakpoints Sep 7, 2017
@Izhaki
Copy link
Contributor Author

Izhaki commented Sep 7, 2017

@rosskevin Sorry, the title was indeed confusing; I've updated it.

Essentially, if there's more than one child, Hidden will render none, regardless of the breakpoint set.

rosskevin added a commit to alienfast/material-ui that referenced this issue Sep 7, 2017
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Sep 7, 2017
rosskevin added a commit that referenced this issue Sep 8, 2017
…8082)

* [Hidden] change children type and add children tests

* Fixes #8072

* use untilSelector for browser tests

* Use strictEqual and ensure no additional unsupported props are passed

* Add back ref check and switch message to keys only to avoid mismatch of message and code checks to debug CI error.

* remove ref, we don’t apply it.

* ok, add back ref?

* codecov hanging, trigger another build

* lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
Development

No branches or pull requests

3 participants