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

[flow] Collapse theme is not an external prop #8470

Merged
merged 1 commit into from Sep 30, 2017

Conversation

rosskevin
Copy link
Member

No description provided.

@oliviertassinari
Copy link
Member

I don't follow. This is not a default property.

@rosskevin
Copy link
Member Author

It's injected by a HOC, not required for a component user to pass in.

@oliviertassinari
Copy link
Member

But the property is required for the component to work. I think that being required by the users is another topic, no?

@rosskevin
Copy link
Member Author

This is the exact pattern for flow use in the entire codebase. DefaultProps & Props == internal props.

export Props represents user props only
DefaultProps contains HOC injected and defaultProps internal to the component.

@rosskevin
Copy link
Member Author

This is also why you see classes located in DefaultProps for example

@oliviertassinari
Copy link
Member

This is the exact pattern for flow use in the entire codebase.

@rosskevin Oh, I have missed this. I'm gonna change the whole code base. At some point, we might be supporting a naked UI version of the components, without the HOCs. If we remove the HOCs from the equation, I can't think of any reason why it shouldn't be required.

@oliviertassinari oliviertassinari added the on hold There is a blocker, we need to wait label Sep 30, 2017
@oliviertassinari oliviertassinari self-assigned this Sep 30, 2017
@rosskevin
Copy link
Member Author

FYI - Technically, now that HOCs are correct, instead of adding classes and theme here, we could be importing types from the HOCs and intersecting them here.

@rosskevin
Copy link
Member Author

Also, before you change the pattern for naked, let me know and let's try one. In our apps, I import Props from here to build bigger components. Internally that prop is required, but externally with HOC the result should be the same.

@oliviertassinari
Copy link
Member

I have been opening #8477.

@rosskevin
Copy link
Member Author

So I'm going to merge this as-is, because the entire codebase is this way and it is not worth leaving open. We'll pick up the changes in your other PR.

@rosskevin rosskevin merged commit 842c426 into mui:v1-beta Sep 30, 2017
@rosskevin rosskevin deleted the flow-collapse branch September 30, 2017 15:17
@rosskevin rosskevin removed the on hold There is a blocker, we need to wait label Sep 30, 2017
@oliviertassinari
Copy link
Member

Nooo, look at the yarn docs:api output...

@oliviertassinari
Copy link
Member

+| <span style="color: #31a148">theme *</span> | Object |  |  |

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants