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

[docs] Plan the breaking changes before v1 #10348

Merged
merged 2 commits into from
Feb 21, 2018

Conversation

oliviertassinari
Copy link
Member

More and more people are trying out v1-beta. It's important to engage the community in the breaking changes process.

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Feb 19, 2018
ROADMAP.md Outdated
Not only does the Material-UI project have to be upgraded for each breaking change,
but we also have to upgrade our own projects.
**We don't take making breaking changes lightly**, it's very costly with UI components.
The test coverage on userland is hard to raise without tools like [visual regression tests](https://www.argos-ci.com/mui-org/material-ui).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"For users, test coverage is hard to raise..."

@oliviertassinari oliviertassinari self-assigned this Feb 20, 2018
@mui mui deleted a comment from mbrookes Feb 20, 2018
@mui mui deleted a comment from mbrookes Feb 20, 2018
+<Grid spacing={8} />
```

- [ ] Flatten the import path [#9532](https://github.com/mui-org/material-ui/issues/9532).
Copy link
Member Author

@oliviertassinari oliviertassinari Feb 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will recycle an old codemode for this breaking.
I won't have the courage to do it by hand on my codebase.
Better scaling it for everybody.
https://github.com/mui-org/material-ui/tree/v1-beta/packages/material-ui-codemod#import-path.

+import Button from '@material-ui/core/Button';
```

- [ ] Look into the Render Props API over the Component Injection API. This one is an area of investigation. For instance, there is potential for simplifying the customization of the transitions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I really love the Component Injection API. Most libraries recently implement a mix of the two by letting the developer use either a render prop or a component prop. Most also actually implement the Component Injection API while still using render as the prop name.

For example see how react-tunnels actually uses the render prop.

Copy link
Member Author

@oliviertassinari oliviertassinari Feb 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/final-form/react-final-form#rendering

Method How it is rendered
component prop (CI) return React.createElement(this.props.component, props)
render prop return this.props.render(props)
a render function as children return this.props.children(props)

Some thoughts:

  • The component injection API (CI) has one important limitation ⚠️. When the parent renders, you create a new component, which unmounts all the children. This force you to move the component creation outside of the render method. It's friction. We have documented it in: https://material-ui.com/guides/composition/#component-property

  • I think that we should encourage people using "a render function as children" over "render prop". It's the same move React did with the new Context API. I'm sure they have made this design choice carefully. I don't know their motivations but mines are:

    • the closing </ helps with code readability.
    • the render property is unnecessary boilerplate.

    It doesn't mean we can't support both.

  • "component prop" and "a render function as children" have both interesting use cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I really love is the terseness of component="nav" instead of render={props => <nav {...props} />} or …> {props => <nav {...props} />} </….

I’d be totally ok if you moved to an hybrid.

What would happen if both component and a render prop is passed?

Copy link
Member Author

@oliviertassinari oliviertassinari Feb 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaking change point was way more about the transition property and Actions property of the Table than revisiting the problem solved by the root component property.

@oliviertassinari oliviertassinari merged commit f73953a into mui:v1-beta Feb 21, 2018
@oliviertassinari oliviertassinari deleted the docs-breaking-changes branch February 21, 2018 19:46
@TroySchmidt
Copy link
Contributor

So excited this is coming out of beta! I think all the breaking changes make sense and will only help the quality. The CSS one seems a little confusing to me, but I am sure once I see more documentation on it, it will make more sense. Great job!

@pureliumy
Copy link

These changes make it more clear, especially love the CSS one. But it still has many limitations in CSS area with JSS, we can't delete the empty checked class for instance. Also I can't write my own keyframe animation and make it work in component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants