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
[typescript] Upgrade types, use string index fallback for CSSProperties to allow nested pseudos #11007
[typescript] Upgrade types, use string index fallback for CSSProperties to allow nested pseudos #11007
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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> { | ||
// Allow pseudo selectors and media queries | ||
[k: string]: CSS.Properties<number | string>[keyof CSS.Properties] | CSSProperties; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't Also doesn't that make this valid?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's necessary for the string index to be compatible with the non-
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>, | ||
|
@@ -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, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could the test include a media query with css props? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, but it's not testing anything different. |
||
}, | ||
}, | ||
})) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 🤷♂️