-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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] Change children type to allow many and add children tests #8082
Conversation
codecov appears hung. It's been about 25 minutes since it should have queued there, but their status says operational. https://status.codecov.io/ |
src/Hidden/HiddenCss.spec.js
Outdated
}); | ||
|
||
it('should work when ChildrenArray', () => { | ||
shallow( |
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.
We should assert the output dom.
I feel like we are missing a part in the story. As far as I know the CSS version do not support multiple children. |
Perhaps this would be better served by creating a My concern is this case: <HiddenCss mdUp>
<Foo />
<Foo />
foo
</HiddenCss>, Which I think would still show string return React.Children.map(children, child => {
if (React.isValidElement(child)) {
return React.cloneElement(child, {
className: classNames(child.props.className, className.join(' ')),
});
}
return child;
}); I guess we could look for Thoughts? |
We could try this approach. We would need to make sure the responsive drawers of the docs are still working. It's not guaranteed. |
@oliviertassinari it looks like the |
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.
The docs seems ok. Good job 👍
return React.cloneElement(children, { | ||
className: classNames(children.props.className, className.join(' ')), | ||
}); | ||
return <span className={className}>{children}</span>; |
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.
What do you think of adding a warning if ...other
is non empty?
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'll alter the current warning, since a ref is no longer valid there.
src/Hidden/HiddenCss.spec.js
Outdated
@@ -25,7 +27,11 @@ describe('<HiddenCss />', () => { | |||
<div className="foo" /> | |||
</HiddenCss>, | |||
); | |||
assert.strictEqual(wrapper.props().className, `foo ${classes.onlySm}`); | |||
assert.strictEqual(wrapper.type(), 'span'); | |||
assert.isTrue(wrapper.hasClass(classes.onlySm)); |
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.
What do you think of sticking to assert.strictEqual(X, true)
? I have found two other tests file using this pattern. It's almost not used.
I hate when this happens. All green locally, remotely, not so much. |
…of message and code checks to debug CI error.
Wow, the simple change that I thought would never end! |
Closes #8072