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

[examples] Fix withRoot accepting any props #12713

Merged
merged 1 commit into from
Aug 30, 2018

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Aug 30, 2018

As a followup to #12712 this includes actual props which revealed that the previous work was incomplete. The generic argument was not defined. The sandbox just emits javascript anyway although typescript displays errors.

I also went ahead and switch from type to interface for State which is a recommended tslint rule.

@@ -2,4 +2,4 @@ import * as React from 'react';
import * as ReactDOM from 'react-dom';
import Index from './pages/index';

ReactDOM.render(<Index />, document.querySelector('#root'));
ReactDOM.render(<Index projectName="Hello, World!" />, document.querySelector('#root'));
Copy link
Member

Choose a reason for hiding this comment

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

I think that providing a property here is an edge case. Do we need to complexify the example?

Copy link
Member Author

Choose a reason for hiding this comment

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

A component with props is one of the core use cases of components, no? Especially since this example should be used for issues I think that we should have a proper scaffold for props usage.

So for me this is the most basic usage of components and not some complex example. I would even argue that state and event handling (which is used in the example) is far more complex than simple props.

Providing props at this point is uncommon, yes. We could refactor this to have a dumb wrapper around index but I can't see any benefit.

Copy link
Member

Choose a reason for hiding this comment

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

Providing props at this point is uncommon

It's exactly my concern. I think that it's better to support it but not actually implementing it.

@oliviertassinari oliviertassinari merged commit 92af52c into mui:master Aug 30, 2018
@oliviertassinari
Copy link
Member

@eps1lon Thanks for the follow up on #12712.

@eps1lon eps1lon deleted the fix-cra-ts-example branch August 30, 2018 15:47
marcelpanse pushed a commit to marcelpanse/material-ui that referenced this pull request Oct 2, 2018
@zannager zannager added the examples Relating to /examples label Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples Relating to /examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants