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

[Box] Test props to attributes forwarding #15365

Merged
merged 2 commits into from Apr 17, 2019

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Apr 16, 2019

This currently creates invalid HTML. Not sure if desired or not but the behavior should be documented in some form (e.g. a test).

Box does not forward the props as DOM attributes.

@eps1lon eps1lon added test component: Box The React component. labels Apr 16, 2019
@oliviertassinari
Copy link
Member

It's a bug.

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 16, 2019

No bundle size changes comparing db67bf2...5f13632

Generated by 🚫 dangerJS against 5f13632

@eps1lon eps1lon added the bug 🐛 Something doesn't work label Apr 16, 2019
@eps1lon eps1lon removed the bug 🐛 Something doesn't work label Apr 16, 2019
@eps1lon
Copy link
Member Author

eps1lon commented Apr 16, 2019

Turns out I was running the wrong test suite. @material-ui/core/Box actually works as expected. The examples at https://next--material-ui.netlify.com/system/basics/ only create invalid HTML.

@oliviertassinari
Copy link
Member

@eps1lon It's just a bug in styled-components then?

@eps1lon
Copy link
Member Author

eps1lon commented Apr 16, 2019

@eps1lon It's just a bug in styled-components then?

I mean not really. Why should styled-components decide what props to forward and which to block? It's following the same behavior that react changed in v16: Forward every prop to the host instance without a warning instead of issuing a warning and blocking those props.

We should probably show in the examples how this could be avoided.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 16, 2019

Why should styled-components decide what props to forward and which to block?

They do it to ease the DX. It's arbitrary, they are using an emotion package actually.

@eps1lon
Copy link
Member Author

eps1lon commented Apr 16, 2019

Why should styled-components decide what props to forward and which to block?

They do it to ease the DX. It's arbitrary, they are using an emotion package actually.

Since this issue is still open I don't think this is expected behavior. Sounds like they do forward everything.

@oliviertassinari
Copy link
Member

@eps1lon Should we close the pull request?

@eps1lon
Copy link
Member Author

eps1lon commented Apr 16, 2019

@eps1lon Should we close the pull request?

I would keep it as a regression test. I'd have to dig more into styled-components and style function but in the current state I wouldn't recommend using our style functions with styled-components because they pollute the DOM.

@oliviertassinari
Copy link
Member

@eps1lon The problem impacts styled-system too https://codesandbox.io/s/kxwwqkrkmv. I'm not aware of any alternative than to write a custom filter logic like we do in Material-UI/Box.

@eps1lon
Copy link
Member Author

eps1lon commented Apr 16, 2019

Well then even better we put a test for this behavior in place. Sounds like this was deliberately implemented but not actually tested.

@eps1lon eps1lon merged commit 8c92321 into mui:next Apr 17, 2019
@eps1lon eps1lon deleted the test/system/dom-attributes branch April 17, 2019 08:40
jztang pushed a commit to nyu-ossd-s19/material-ui that referenced this pull request Apr 21, 2019
* [Box] Test props to attributes forwarding

* correct tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Box The React component. test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants