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

[typescript] Upgrade types, use string index fallback for CSSProperties to allow nested pseudos #11007

Merged
merged 1 commit into from Apr 14, 2018
Merged

[typescript] Upgrade types, use string index fallback for CSSProperties to allow nested pseudos #11007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions package.json
View file
Open in desktop
Expand Up @@ -68,7 +68,7 @@
},
"dependencies": {
"@types/jss": "^9.3.0",
"@types/react-transition-group": "^2.0.6",
"@types/react-transition-group": "^2.0.8",
"babel-runtime": "^6.26.0",
"brcast": "^3.0.1",
"classnames": "^2.2.5",
Expand Down Expand Up @@ -98,7 +98,7 @@
},
"devDependencies": {
"@types/enzyme": "^3.1.4",
"@types/react": "16.3.4",
"@types/react": "16.3.9",
"argos-cli": "^0.0.9",
"autoprefixer": "^8.0.0",
"autosuggest-highlight": "^3.1.1",
Expand Down Expand Up @@ -196,7 +196,7 @@
"workbox-build": "^3.0.1"
},
"resolutions": {
"@types/react": "16.3.4"
"@types/react": "16.3.9"
},
"sideEffects": false,
"nyc": {
Expand Down
8 changes: 7 additions & 1 deletion src/styles/withStyles.d.ts
View file
Open in desktop
@@ -1,5 +1,11 @@
import * as React from 'react';
import { Theme } from './createMuiTheme';
import * as CSS from 'csstype';

export interface CSSProperties extends CSS.Properties<number | string> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a new name that doesn't get confused with React.CSSProperties would be nice

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice, although it would be a breaking change 🤷‍♂️

// Allow pseudo selectors and media queries
[k: string]: CSS.Properties<number | string>[keyof CSS.Properties] | CSSProperties;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't CSS.Properties<number | string>[keyof CSS.Properties] already covered by extends CSS.Properties<number | string>?

Also doesn't that make this valid?

color: {
  color: 'red';
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't CSS.Properties<number | string>[keyof CSS.Properties] already covered by extends CSS.Properties<number | string>?

It's necessary for the string index to be compatible with the non-string properties. If you remove that it will be a type error.

Also doesn't that make this valid?

color: {
 color: 'red';
}

Allowing an arbitrary string index for nested pseudo selectors and media queries unavoidably means that invalid properties can get through. No way around it.

}

/**
* This is basically the API of JSS. It defines a Map<string, CSS>,
Expand All @@ -8,7 +14,7 @@ import { Theme } from './createMuiTheme';
* - the `keys` are the class (names) that will be created
* - the `values` are objects that represent CSS rules (`React.CSSProperties`).
*/
export type StyleRules<ClassKey extends string = string> = Record<ClassKey, React.CSSProperties>;
export type StyleRules<ClassKey extends string = string> = Record<ClassKey, CSSProperties>;

export type StyleRulesCallback<ClassKey extends string = string> = (
theme: Theme,
Expand Down
9 changes: 9 additions & 0 deletions test/typescript/styles.spec.tsx
View file
Open in desktop
Expand Up @@ -166,3 +166,12 @@ const DecoratedComponent = withStyles(styles)(

// no 'classes' property required at element creation time (#8267)
<DecoratedComponent text="foo" />;

// Allow nested pseudo selectors
withStyles<'listItem'>(theme => ({
listItem: {
'&:hover $listItemIcon': {
visibility: 'inherit',
Copy link
Contributor

Choose a reason for hiding this comment

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

could the test include a media query with css props?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, but it's not testing anything different.

},
},
}))
18 changes: 14 additions & 4 deletions yarn.lock
View file
Open in desktop
Expand Up @@ -128,11 +128,17 @@
dependencies:
"@types/react" "*"

"@types/react@*", "@types/react@16.3.4":
version "16.3.4"
resolved "https://registry.yarnpkg.com/@types/react/-/react-16.3.4.tgz#9bbb301cd38270ae200bed329a342bd2f140c3ea"
"@types/react-transition-group@^2.0.8":
version "2.0.8"
resolved "https://registry.yarnpkg.com/@types/react-transition-group/-/react-transition-group-2.0.8.tgz#1ea86f6d8288e4bba8d743317ba9cc61cdacc1ad"
dependencies:
csstype "^2.0.0"
"@types/react" "*"

"@types/react@*", "@types/react@16.3.9":
version "16.3.9"
resolved "https://registry.yarnpkg.com/@types/react/-/react-16.3.9.tgz#15be16b53c89aac558cf6445774502b2792555ff"
dependencies:
csstype "^2.2.0"

"@zeit/check-updates@1.1.1":
version "1.1.1"
Expand Down Expand Up @@ -2946,6 +2952,10 @@ csstype@^2.0.0:
version "2.1.1"
resolved "https://registry.yarnpkg.com/csstype/-/csstype-2.1.1.tgz#37b01a7a9958ef0b88167ff6678deccd732e0ae2"

csstype@^2.2.0:
version "2.2.0"
resolved "https://registry.yarnpkg.com/csstype/-/csstype-2.2.0.tgz#1656ef97553ac53b77090844a2531c6660ebd902"

currently-unhandled@^0.4.1:
version "0.4.1"
resolved "https://registry.yarnpkg.com/currently-unhandled/-/currently-unhandled-0.4.1.tgz#988df33feab191ef799a61369dd76c17adf957ea"
Expand Down