Skip to content

[core] Standardize the component injection pattern#11204

Merged
oliviertassinari merged 1 commit into
mui:v1-betafrom
oliviertassinari:Component-property
May 3, 2018
Merged

[core] Standardize the component injection pattern#11204
oliviertassinari merged 1 commit into
mui:v1-betafrom
oliviertassinari:Component-property

Conversation

@oliviertassinari

@oliviertassinari oliviertassinari commented May 1, 2018

Copy link
Copy Markdown
Member

I couldn't find a clean way to support the render props pattern.
Doing such would require to greatly reduce the usage of JSX. It would really harm source code readability.

Instead, I have been focusing on standardizing our component injection story.
This way, we can go back to the render props after stable v1 is released and see if source code readability worth be sacrificed for the render prop pattern.

Closes #10476. Might be better to start from a fresh issue in the future.

Breaking change

<Tabs
- TabScrollButton={TabScrollButtonWrapped}
+ ScrollButtonComponent={TabScrollButtonWrapped}
<TablePagination
- Actions={TablePaginationActionsWrapped}
+ ActionsComponent={TablePaginationActionsWrapped}
<Dialog
- transition={Transition}
+ TransitionComponent={Transition}
<Menu
- transition={Transition}
+ TransitionComponent={Transition}
<Snackbar
- transition={Transition}
+ TransitionComponent={Transition}
<Popover
- transition={Transition}
+ TransitionComponent={Transition}
<StepContent
- transition={Transition}
+ TransitionComponent={Transition}

@oliviertassinari oliviertassinari added the on hold There is a blocker, we need to wait. label May 1, 2018
@oliviertassinari oliviertassinari added core breaking change Introduces changes that are not backward compatible. and removed on hold There is a blocker, we need to wait. labels May 1, 2018
@oliviertassinari oliviertassinari self-assigned this May 1, 2018
@oliviertassinari
oliviertassinari requested a review from a team May 1, 2018 20:56
@mbrookes

mbrookes commented May 1, 2018

Copy link
Copy Markdown
Member

The only question I have (and I don't have a strong opinion) is whether the first character of the prop should be capitalised or not?

@oliviertassinari

oliviertassinari commented May 1, 2018

Copy link
Copy Markdown
Member Author

We also have https://github.com/mui-org/material-ui/blob/3e4854b39b17511d9b312071ca93061eeb503f5d/packages/material-ui/src/Input/Input.js#L542.
We also have the xxxProps where xxx is capitalized based on the component name, e.g. InputProps and inputProps.

@kgregory

kgregory commented May 1, 2018

Copy link
Copy Markdown
Collaborator

The only question I have (and I don't have a strong opinion) is whether the first character of the prop should be capitalised or not?

This is a compliment to the existing component property, which pertains to the root element. I feel like these props should follow a lowerCamelCase naming convention.

@rosskevin

Copy link
Copy Markdown
Contributor

If this is the direction to go, the property name should remain capitalized:

  1. to be consistent with cases such as inputProps vs InputProps
  2. stops the destructuring aliasing needed such as
const { transitionComponent: TransitionComponent } = this.props
return <TransitionComponent/>

@kgregory

kgregory commented May 1, 2018

Copy link
Copy Markdown
Collaborator

stops the destructuring aliasing needed such as...

This is a good point. Maybe we should go further and change component to Component for consistency and to eliminate the destructuring.

@oliviertassinari

oliviertassinari commented May 2, 2018

Copy link
Copy Markdown
Member Author

@kgregory The logics I have been following:

  • destructuring aliasing wasn't a concern. It's better if we can avoid it, but I don't think it will matter in the long run if we execute on a versatile element customization story: support for component injection <XXXComponent />, render prop XXXrenderer() and element React.cloneElement(XXXnode)). Something we could fit into a single property name: all-inclusive.
  • I like how the component suffix makes it obvious that it's a property to mess with the React structure. You don't need to go back to the property list API to see it.
  • I have tried to follow a components?: Object structure like the classes. Ultimately, I reverted. The nesting wasn't providing me any additional value.
  • The capitalization was added to stay consistent with the existing XXXProps properties. I don't have a strong feeling in either way. I have picked the solution requiring the less breaking changes possible.

@oliviertassinari
oliviertassinari merged commit 9747089 into mui:v1-beta May 3, 2018
@oliviertassinari
oliviertassinari deleted the Component-property branch May 3, 2018 19:06
@franklixuefei

Copy link
Copy Markdown
Contributor

@oliviertassinari You left out Snackbar.d.ts :-D
On it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Introduces changes that are not backward compatible.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants