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

pelotom · 2018-04-12T22:45:18Z

Resolves #11004.

My PR to @types/react to make CSSProperties a closed type (sans string index fallback) indirectly broke Material UI, which was relying on it. The problem is that React's CSSProperties are only meant to capture what is expressible by inline styles, but Material UI's JSS API allows nested pseudo selectors, for example. So we need to make a new CSSProperties specific to Material UI, based on CSSType, which allows a string index.

…ested pseudos

franklixuefei · 2018-04-13T00:05:27Z

So, from this on, we should be importing CSSProperties from material-ui rather than using React.CSSProperties, right?

pelotom · 2018-04-13T00:12:45Z

@franklixuefei in the context of writing styles for use with withStyles, yes. Only use React.CSSProperties in the context of inline styles.

franklixuefei · 2018-04-13T00:33:59Z

@pelotom Thanks. There can't be nested styles or psuedo selectors in inline styles of course... I understand it now.

varoot · 2018-04-13T14:19:01Z

Material UI also supports array syntax and default unit (e.g. margin: [[2, 8]] translates to margin: 2px 8px) which is now broken with the new typings. Can we also fix this?

pelotom · 2018-04-13T17:37:12Z

@varoot

Material UI also supports array syntax and default unit (e.g. margin: [[2, 8]] translates to margin: 2px 8px) which is now broken with the new typings. Can we also fix this?

Interesting, I wasn't aware of this. Is the outer array used for fallbacks?

It looks like this only worked before with the old anything-goes React.CSSProperties because margin was defined as

        margin?: CSSWideKeyword | any;

varoot · 2018-04-13T23:19:53Z

According to JSS documentation, the outer array represents comma-separated values and the inner one for space-separated values.

Apparently JSS also accepts callback function as value but I'm not sure about whether Material UI supports this or how to use it.

pelotom · 2018-04-13T23:56:46Z

According to JSS documentation, the outer array represents comma-separated values and the inner one for space-separated values.

Hm, currently @types/jss doesn't support these things, I guess that's a change that needs to be made there.

Apparently JSS also accepts callback function as value but I'm not sure about whether Material UI supports this or how to use it.

Yeah, in general I'm not sure which things from JSS are supported by Material UI and which not. My understanding was that it was a strict subset somehow, but I don't know exactly what the differences are. Maybe @oliviertassinari could shed some light on this?

pelotom · 2018-04-14T01:30:23Z

I’m working on a PR for @types/jss to make all the examples in that link work, @varoot. There are lots of deficiencies currently. Then we can use those types (or whatever subset of them makes sense) in MUI. Until then I think this PR is good to merge as an intermediate improvement.

oliviertassinari · 2018-04-14T14:46:02Z

Material UI also supports array syntax and default unit (e.g. margin: [[2, 8]] translates to margin: 2px 8px) which is now broken with the new typings. Can we also fix this?

@varoot Material-UI doesn't support it out of the box. This feature is coming from: jss-expand. A JSS plugin that doesn't come by default (the plugin cost is 1.3 kB gzipped).

Apparently JSS also accepts callback function as value but I'm not sure about whether Material UI supports this or how to use it.

No, we don't support it yet. Not supporting it allows a better cache strategy and a lighter bundle.

@pelotom The pull-request looks good to me. Any blocker for merging it?

pelotom · 2018-04-14T15:34:27Z

@oliviertassinari Thanks for the info. This is good to merge.

varoot · 2018-04-14T15:55:20Z

@oliviertassinari I could use the syntax without installing any other plugin so I'm pretty sure this is JSS's standard syntax (as documented on their Core JSON Syntax page). JSS Expand seems to offer different syntax (only one level of array needed and allowing object syntax).

I have a repository to prove it but you can also try it yourself. But since people don't seem to be aware of this feature, maybe I just shouldn't use it.

oliviertassinari · 2018-04-14T16:06:05Z

@varoot I have never used it but I can trust you on this.

xaviergonz

looks good mostly

xaviergonz · 2018-04-12T22:55:02Z

src/styles/withStyles.d.ts

@@ -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> {


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

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

xaviergonz · 2018-04-12T22:55:37Z

test/typescript/styles.spec.tsx

+withStyles<'listItem'>(theme => ({
+  listItem: {
+    '&:hover $listItemIcon': {
+      visibility: 'inherit',


could the test include a media query with css props?

Sure, but it's not testing anything different.

xaviergonz · 2018-04-14T16:21:07Z

src/styles/withStyles.d.ts

+
+export interface CSSProperties extends CSS.Properties<number | string> {
+  // Allow pseudo selectors and media queries
+  [k: string]: CSS.Properties<number | string>[keyof CSS.Properties] | CSSProperties;


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'; }

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.

pelotom · 2018-04-14T19:42:59Z

@varoot

@oliviertassinari I could use the syntax without installing any other plugin so I'm pretty sure this is JSS's standard syntax (as documented on their Core JSON Syntax page). JSS Expand seems to offer different syntax (only one level of array needed and allowing object syntax).

I have a repository to prove it but you can also try it yourself. But since people don't seem to be aware of this feature, maybe I just shouldn't use it.

It definitely seems to work, and I don't see why it shouldn't be supported by the typings!

From my further experimentation it looks like the function and observable options from that page are not supported by Material UI, probably because link: true isn't being specified? Though I think there has been some talk of someday allowing styles to depend on props using the function mechanism.

pelotom requested a review from rosskevin April 12, 2018 22:45

pelotom mentioned this pull request Apr 12, 2018

[TS] From @types/react 16.3.9 React.CSSProperties is strongly typed #11004

Closed

Upgrade types, use string index fallback for CSSProperties to allow n…

fa06fe4

…ested pseudos

pelotom force-pushed the add-string-index-to-cssproperties branch from 79acc2d to fa06fe4 Compare April 12, 2018 22:48

pelotom added the typescript label Apr 12, 2018

pelotom changed the title ~~Upgrade types, use string index fallback for CSSProperties to allow nested pseudos~~ [typescript] Upgrade types, use string index fallback for CSSProperties to allow nested pseudos Apr 12, 2018

pelotom mentioned this pull request Apr 12, 2018

[React] Remove string index fallback for CSS properties DefinitelyTyped/DefinitelyTyped#24911

Merged

9 tasks

rosskevin approved these changes Apr 13, 2018

View reviewed changes

pelotom merged commit 5ee6676 into mui:v1-beta Apr 14, 2018

pelotom deleted the add-string-index-to-cssproperties branch April 14, 2018 15:34

xaviergonz reviewed Apr 14, 2018

View reviewed changes

rosskevin mentioned this pull request Apr 25, 2018

[typescript] allow pseudos on any theme mixins #11145

Merged

eps1lon mentioned this pull request Aug 30, 2018

[typescript] createStyles is incompatible with top level @media queries #12277

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[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

pelotom commented Apr 12, 2018

franklixuefei commented Apr 13, 2018

pelotom commented Apr 13, 2018

franklixuefei commented Apr 13, 2018

varoot commented Apr 13, 2018

pelotom commented Apr 13, 2018

varoot commented Apr 13, 2018

pelotom commented Apr 13, 2018

pelotom commented Apr 14, 2018

oliviertassinari commented Apr 14, 2018 •

edited

Loading

pelotom commented Apr 14, 2018

varoot commented Apr 14, 2018

oliviertassinari commented Apr 14, 2018

xaviergonz left a comment

xaviergonz Apr 12, 2018

pelotom Apr 14, 2018

xaviergonz Apr 12, 2018

pelotom Apr 14, 2018

xaviergonz Apr 14, 2018 •

edited

Loading

pelotom Apr 14, 2018

pelotom commented Apr 14, 2018 •

edited

Loading

[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

Conversation

pelotom commented Apr 12, 2018

franklixuefei commented Apr 13, 2018

pelotom commented Apr 13, 2018

franklixuefei commented Apr 13, 2018

varoot commented Apr 13, 2018

pelotom commented Apr 13, 2018

varoot commented Apr 13, 2018

pelotom commented Apr 13, 2018

pelotom commented Apr 14, 2018

oliviertassinari commented Apr 14, 2018 • edited Loading

pelotom commented Apr 14, 2018

varoot commented Apr 14, 2018

oliviertassinari commented Apr 14, 2018

xaviergonz left a comment

Choose a reason for hiding this comment

xaviergonz Apr 12, 2018

Choose a reason for hiding this comment

pelotom Apr 14, 2018

Choose a reason for hiding this comment

xaviergonz Apr 12, 2018

Choose a reason for hiding this comment

pelotom Apr 14, 2018

Choose a reason for hiding this comment

xaviergonz Apr 14, 2018 • edited Loading

Choose a reason for hiding this comment

pelotom Apr 14, 2018

Choose a reason for hiding this comment

pelotom commented Apr 14, 2018 • edited Loading

oliviertassinari commented Apr 14, 2018 •

edited

Loading

xaviergonz Apr 14, 2018 •

edited

Loading

pelotom commented Apr 14, 2018 •

edited

Loading