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

[system] Fix breakpoints typescript types #15720

Merged
merged 13 commits into from
Jun 3, 2019
Merged

Conversation

Kujawadl
Copy link
Contributor

Changes

Box prop types previously didn't allow for responsive values (e.g. arrays/objects) and also did not account for variables in fontWeight.

fontWeight is now a string | number value to accept variable names without compiler errors, as opposed to the union type defined in React.CSSProperties.

Necessity

Without these changes, the following uses in typescript would have compile errors:

{/* "fontWeightBold" is not a valid value of React.CSSProperties["fontWeight"] */}
<Box fontWeight="fontWeightBold">
  ...
</Box>

{/* Type { ... } is not assignable to type string | number */}
<Box m={{ xs: 1, sm: 2, md: 3 }}>
...
</Box>

There are probably other props that need updating, but fontWeight was the only one that I found that seems to have variables that would actually be used. The others are:

flexDirection
flexGrow
flexShrink
flexWrap
overflowX
overflowY
position
textAlign
zIndex

All of these have union types, e.g. "column" | "column-reverse" | "row" | "row-reverse" | ..., and I can't think of any variables that would be used, so it's probably better to keep the union types than lose them for the sake of variables that aren't likely to be used.

Alternative Options

I would have just created a new union type for fontWeight, something like:

fontWeight?:
  | React.CSSProperties['fontWeight']
  | 'fontWeightLight'
  | 'fontWeightRegular' 
  | ...

Except that would prevent users from using custom variables, e.g. in our codebase we have a fontWeightExtraBold. The solution to that would be to create and export a named type, e.g. FontWeightValue and set it to the above. I think that's the most complete solution, but starts to get tedious to maintain, so I opted for simple here.

@mui-pr-bot
Copy link

mui-pr-bot commented May 16, 2019

No bundle size changes comparing d533b46...fa1bed2

Generated by 🚫 dangerJS against fa1bed2

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.

This looks nice so far but is missing tests.

Could you link to a docs section that explains this API? Otherwise it would be nice to add a demo that also has a TypeScript version (which would act as a test).

packages/material-ui/src/Box/Box.d.ts Outdated Show resolved Hide resolved
@Kujawadl
Copy link
Contributor Author

Okay, a few more changes here, that was quite the rabbit hole to go down. I started converting docs demos to typescript (for testing) and found that there were some more type changes that needed to be made and that weren't being tested.

Some notes about the recent changes:

  • My eslint plugin was throwing errors about jsx in typescript, so I added .tsx files to the acceptable files list. This doesn't affect anything practically, but silences IDE warnings for anybody who's using an IDE with eslint integration. I can revert this if you'd prefer, I just figured it's useful for anybody else in my situation.
  • The breakpoints function in material-ui-system was typed to accept a style function whose props already has the breakpoints, and then return the same type of style function. It should be that it takes a non-breakpoint-related style function (e.g. typography) and returns a style function with the breakpoints added in. I had to fix this to get the collocation doc demo working without tslint errors.
  • I added a link in the ResponsiveValue type's jsdoc annotation to the responsive page that shows demos of these responsive props in action. If you don't want jsdoc annotations, I can revert this commit.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Nice!

docs/src/pages/system/basics/ArrayApi.js Outdated Show resolved Hide resolved
docs/src/pages/system/basics/basics.md Outdated Show resolved Hide resolved
docs/src/pages/system/basics/basics.md Outdated Show resolved Hide resolved
docs/src/pages/system/basics/ArrayApi.js Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari added the package: system Specific to @mui/system label May 18, 2019
@oliviertassinari oliviertassinari added 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 May 19, 2019
@Kujawadl
Copy link
Contributor Author

Concerning #15548: I don't think that PR addresses the same issues as this one, but if @merceyz doesn't have time to continue working on it I can take a look later today or tomorrow as it's also something my team would find useful. Just let me know.

@oliviertassinari oliviertassinari changed the base branch from next to master May 23, 2019 21:08
@eps1lon
Copy link
Member

eps1lon commented May 29, 2019

I'm sorry I forgot this was here. Could you rebase this with master without changing types implementation and see if the demos work? #15884 might've fixed this already.

Dylan Jager-Kujawa added 11 commits June 1, 2019 16:53
Box prop types previously didn't allow for responsive values (e.g. arrays/objects) and also did not account for variables in fontWeight value.

fontWeight is now a string value to accept variable names without compiler errors, as opposed to the union type defined in React.CSSProperties.
Accepts a generic argument for the child function(s) prop types and return a style function with the breakpoints prop types and child prop types
* Re-add CSS output info comments
* Reduce number of breakpoints from 5 to 3
* Remove component name duplication
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 1, 2019

@eps1lon I have rebased the pull request maintaining the failing case:

import React from 'react';
import { typography, breakpoints } from '@material-ui/system';
import styled from 'styled-components';

const Box = styled.div`
  ${breakpoints(typography)}
`;

/**
 * Outputs:
 *
 * font-size: 12px;
 * @media (min-width: 600px) {
 *   font-size: 18px;
 * }
 * @media (min-width: 960px) {
 *   font-size: 24px;
 * }
 */
export default function CollocationApi() {
  return (
    <Box xs={{ fontSize: 12 }} sm={{ fontSize: 18 }} md={{ fontSize: 24 }}>
      Collocation API
    </Box>
  );
}

$ tslint -p docs/tsconfig.json

ERROR: /tmp/material-ui/docs/src/pages/system/basics/CollocationApi.tsx:2:35 - TypeScript@next compile error:
Module '"../../../../../packages/material-ui-system/src"' has no exported member 'BreakpointsProps'.
ERROR: /tmp/material-ui/docs/src/pages/system/basics/CollocationApi.tsx:6:17 - TypeScript@next compile error:
Argument of type 'StyleFunction<Partial<Record<"fontFamily" | "fontSize" | "fontWeight" | "textAlign", any>>>' is not assignable to parameter of type 'StyleFunction<{ theme: { breakpoints?: unknown; }; }>'.
Types of parameters 'props' and 'props' are incompatible.
Type '{ theme: { breakpoints?: unknown; }; }' has no properties in common with type 'Partial<Record<"fontFamily" | "fontSize" | "fontWeight" | "textAlign", any>>'.

@eps1lon eps1lon self-requested a review June 3, 2019 11:37
@eps1lon eps1lon changed the title [Box] Accept responsive values and variables in Box props [system] Fix breakpoints typescript types Jun 3, 2019
@eps1lon eps1lon 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 Jun 3, 2019
@eps1lon eps1lon merged commit 6f516a5 into mui:master Jun 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: system Specific to @mui/system typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants