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

[styles] Allow ref on withTheme components in TS #17695

Merged
merged 3 commits into from Oct 20, 2019
Merged

Conversation

ianschmitz
Copy link
Contributor

withTheme now forwards refs, but the typing for the ref prop is missing in the declaration file.

@mui-pr-bot
Copy link

mui-pr-bot commented Oct 3, 2019

No bundle size changes comparing 5d564f9...f8e0f19

Generated by 🚫 dangerJS against f8e0f19

@eps1lon eps1lon changed the title Add ref prop to WithTheme [styles] Add ref prop to WithTheme Oct 4, 2019
@eps1lon
Copy link
Member

eps1lon commented Oct 4, 2019

I just realized that this is probably the wrong place. The WithTheme interface should be used for props of components decorated with withTheme.

This would now imply that you have access inside your component to props.ref. However, ref (like key) will be omitted.

interface Props extends WithTheme {}

function Component(props: Props) {
  const {ref} = props; // whoops, this can never happen
}

export default withTheme(Component);

Could you add a usage example that would be allowed with this change (i.e. a test) to https://github.com/mui-org/material-ui/blob/2bcc874cf07b81202968f769cb9c2398c7c11311/packages/material-ui-styles/test/styles.spec.tsx#L70?

@eps1lon eps1lon added package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. typescript PR: needs test labels Oct 4, 2019
@oliviertassinari oliviertassinari removed PR: needs test package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. typescript labels Oct 9, 2019
@oliviertassinari oliviertassinari added package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. PR: needs test typescript PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: needs test package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. typescript PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Oct 9, 2019
@oliviertassinari
Copy link
Member

@ianschmitz Did you had the chance to look at the problem?

@ianschmitz
Copy link
Contributor Author

Oh shoot sorry I lost track of this. I'll take a look today!

`withTheme` now forwards refs, but the typing for the `ref` prop is missing in the declaration file.
@ianschmitz
Copy link
Contributor Author

Alright I've updated it. I think it's what you were after. I've also updated material-ui-styles/src/withTheme/withTheme.d.ts to match.

Not super happy with the typings in material-ui-styles. Let me know if you're okay with it.

@ianschmitz ianschmitz changed the title [styles] Add ref prop to WithTheme [styles] Add ref prop to withTheme Oct 17, 2019
@oliviertassinari oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Oct 17, 2019
@ianschmitz
Copy link
Contributor Author

Also a question. I noticed innerRef is on WithTheme in material-ui-styles/src/withTheme/withTheme.d.ts. If you're happy with the new interface i created (ThemedComponentProps) shall i move it in there? Today there's the same issue @eps1lon identified above.

@eps1lon eps1lon self-requested a review October 17, 2019 18:22
@eps1lon eps1lon added package: material-ui Specific to @mui/material package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. labels Oct 20, 2019
@eps1lon eps1lon merged commit b40a773 into mui:master Oct 20, 2019
@eps1lon eps1lon changed the title [styles] Add ref prop to withTheme [styles] Allow ref on withTheme components in TS Oct 20, 2019
@eps1lon
Copy link
Member

eps1lon commented Oct 20, 2019

@ianschmitz Perfect, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: material-ui Specific to @mui/material package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants