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.

Sign up for GitHub

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

Jump to bottom

[styles] Add types for defaultTheme option in makeStyles #14862

Merged
merged 5 commits into from
Mar 15, 2019
Merged

[styles] Add types for defaultTheme option in makeStyles #14862

merged 5 commits into from
Mar 15, 2019

Conversation

vitkon
Copy link
Contributor

@vitkon vitkon commented Mar 12, 2019

adds defaultTheme option into makeStyles interface (inline with documentation https://material-ui.com/css-in-js/api/)

@vitkon vitkon changed the title defaultTheme option interface for makeStyles [makeStyles] defaultTheme option interface Mar 12, 2019
@vitkon vitkon changed the title [makeStyles] defaultTheme option interface [styles] defaultTheme option interface for makeStyles Mar 12, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Mar 12, 2019 •
edited
Loading

No bundle size changes comparing 89ebedc...3ea5cf7

Generated by 🚫 dangerJS against 3ea5cf7

@eps1lon eps1lon added this to the v4 milestone Mar 12, 2019
@eps1lon eps1lon added typescript package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. labels Mar 12, 2019
@vitkon
Copy link
Contributor Author

vitkon commented Mar 13, 2019

@eps1lon , should be as generic as possible now, Please have a look if this works

@eps1lon
Copy link
Member

eps1lon commented Mar 13, 2019

Could you add a test for a matching defaultTheme and one for a mismatching defaultTheme and $ExpectError. Something like makeStyles(t => someStylesFrom(t), { defaultTheme: t }) and makeStyles(t => someStylesFrom(t), { defaultTheme: notMatchingT }). They're located in packages/material-ui-styles/test/*.tsx?.

eps1lon
eps1lon reviewed Mar 13, 2019
View reviewed changes
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Almost there. Just some formatting left to do.

packages/material-ui-styles/test/index.spec.tsx Outdated Show resolved Hide resolved
@vitkon
Copy link
Contributor Author

vitkon commented Mar 13, 2019 •
edited
Loading

Almost there. Just some formatting left to do.

Thanks for your prompt responses and support, @eps1lon . Much appreciated.
Pushed the changes in, please have a peek.

@vitkon
Merge pull request #1 from mui-org/next
00d18af 
bump changes
@vitkon
defaultTheme option interface for makeStyles
d97fa81 
adds `defaultTheme` option into `makeStyles` interface (inline with documentation https://material-ui.com/css-in-js/api/)

adds test for makeStyles defaultTheme interface
@vitkon
Update index.spec.tsx
c76e3f0
@vitkon
Update index.spec.tsx
bba5bf1
@eps1lon eps1lon self-assigned this Mar 14, 2019
eps1lon
eps1lon requested changes Mar 14, 2019
View reviewed changes
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

I wouldn't recommend developing code in the github ui. Using a proper code editor (like vscode) will already show you any issues while coding. This removes the roundtrip downtime to CI.

packages/material-ui-styles/src/makeStyles/makeStyles.d.ts Show resolved Hide resolved
packages/material-ui-styles/src/withStyles/withStyles.d.ts Outdated Show resolved Hide resolved
packages/material-ui-styles/test/index.spec.tsx Show resolved Hide resolved
@vitkon
Copy link
Contributor Author

vitkon commented Mar 14, 2019

I wouldn't recommend developing code in the github ui. Using a proper code editor (like vscode) will already show you any issues while coding. This removes the roundtrip downtime to CI.

on VSCode normally, but on the go sometimes have to use github UI, sorry.

@eps1lon
Copy link
Member

eps1lon commented Mar 14, 2019

I wouldn't recommend developing code in the github ui. Using a proper code editor (like vscode) will already show you any issues while coding. This removes the roundtrip downtime to CI.

on VSCode normally, but on the go sometimes have to use github UI, sorry.

No worries. Just wanted to save you some time.

@eps1lon
Copy link
Member

eps1lon commented Mar 14, 2019

Alright, looks good. Will merge soon.

@eps1lon eps1lon merged commit 5af2adf into mui:next Mar 15, 2019
@eps1lon eps1lon changed the title [styles] defaultTheme option interface for makeStyles [styles] Add types for defaultTheme option in makeStyles (#14862) Mar 15, 2019
@eps1lon
Copy link
Member

eps1lon commented Mar 15, 2019

@vitkon Thanks for sticking with it. Nice work 👍

@vitkon vitkon deleted the patch-1 branch March 15, 2019 10:29
@oliviertassinari oliviertassinari changed the title [styles] Add types for defaultTheme option in makeStyles (#14862) [styles] Add types for defaultTheme option in makeStyles Mar 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eps1lon eps1lon eps1lon approved these changes

Assignees

@eps1lon eps1lon

Labels
package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. typescript
Projects
None yet
Milestone
v4
Development

Successfully merging this pull request may close these issues.
3 participants
@vitkon @mui-pr-bot @eps1lon