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

RFC: Material-UI v4 breaking changes #13663

Closed
34 of 56 tasks
eps1lon opened this issue Nov 21, 2018 · 50 comments
Closed
34 of 56 tasks

RFC: Material-UI v4 breaking changes #13663

eps1lon opened this issue Nov 21, 2018 · 50 comments
Labels
breaking change component: select This is the name of the generic UI component, not the React module! discussion umbrella For grouping multiple issues to provide a holistic view
Milestone

Comments

@eps1lon
Copy link
Member

eps1lon commented Nov 21, 2018

⚠️ No more breaking changes planned.

Tracking issue for all the breaking changes that are planned or proposed for Material-UI v4.

v4 preview: https://next--material-ui.netlify.com/

Unchecked checkboxes mean planned only. If those changes should be discussed then there either exists an existing issue or a separate one should be opened. Checked means a PR is either open or merged into next.
Changes live in the next branch. Beta releases are not planned yet.

Releases

4.0.0-alpha.0

  • Change UMD name to MaterialUI (PR)
  • Theme, make augmentColor() immutable (issue, PR)
  • Remove deprecated Button variants (see variant prop documentation). (PR)
  • Remove the depreacted Button Fab code. (issue, PR)
  • Drop node 6, node 8 new lowest supported runtime (PR)
  • Remove typings for import Button from '@material-ui/core/es/Button' (issue, PR)
  • Increase React peer dependency version to React >= 16.8.0 (PR)
  • Move away from hard-coded 8px public API. For instance, I think that the following API would be much better <Grid spacing={1|2|3} /> 👍. <Grid spacing={8|16|24} /> 👎. (PR)
  • Remove recompose dependency, use React.memo instead. (PR)

4.0.0-alpha.1

  • Remove /es folder from icons build (PR)
  • [InputLabel] Remove FormLabelClasses in favor of asterisk class (issue, PR)
  • Remove deprecated Typography variants (issue, PR).
  • Typography, rename property headlineMapping -> variantMapping.
    I'm open to better wording, at least, this one makes it obvious it's related to the variant property. (PR)
  • Link: remove block prop. (PR)
    -<Link block />
    +<Link display="block" />
  • withTheme(options)(Component) -> withTheme(Component). There is no need for an options argument. (PR)
  • Typography, change default display from block to initial, less opinionated. (PR)

4.0.0-alpha.2

  • Tab, move the padding CSS from the label to the root.
    The more style we have on the root, the easier it's for people to override the component. (PR)
  • Add Table dense support (PR)

4.0.0-alpha.3

  • forward refs in withStyles (was @material-ui/core/styles/withStyles will be @material-ui/styles/withStyles) (issue, PR)

4.0.0-alpha.4

  • Remove deprecated Divider inset property. (PR)
  • Rename nativeColor -> htmlColor for consistency with htmlFor. (PR)

4.0.0-alpha.5

  • Modal ignore event.defaultPrevented. (issue, PR)

4.0.0-alpha.6

4.0.0-alpha.7

4.0.0-alpha.8

4.0.0-alpha.9

4.0.0-beta.0

🏁

Rejected

  • Move @material-ui/core/styles/createMuiTheme -> @material-ui/core/createMuiTheme
    As we are moving the styling solution into its own package, this folder only focuses on the theme, we can reflect it in the name of the folder. (issue)
  • Typography consitency (comment, PR)
  • Prefix the style helpers that inject a default theme:
    import { muiMakeStyles } from '@material-ui/core/styles';
    import { makeStyles } from '@material-ui/styles';
  • Refactor the Checkbox to work like the native one
    (https://codesandbox.io/s/448wmxwx9, https://github.com/erikras/redux-form-material-ui/blob/v5.0.0-beta-3/src/Checkbox.js).
  • Simplify the server-side rendering API
  • Icon/SvgIcon change the fontSize value property default -> medium.
    This matches the Button size enums and the less often we use default, the better. (PR)
  • Simplify the TextField code by not supporting this edge case:
    https://github.com/mui-org/material-ui/pull/13487/files#r230219558. (PR)
  • Rename the component Grid List -> Image List to follow the specification wording. (PR)
  • Rename theme.palette.type -> theme.palette.variant.
    We use the variant wording all over the place, what's a "type"?
  • Select / NativeSelect use InputBase over Input by default
    Better perf.
  • Remove withMobileDialog
    This helper doesn't provide any value when looking at the implementation. It would be better to educate people using withWidth(), and the upcoming hook API. (PR)
  • Remove withWidth(). Use useMediaQuery instead. I would like to allow people to use custom breakpoints. withWidth is blocking this capability.
  • Input, remove the dead classes that were moved to the InputBase.
  • Input, rename margin?: 'none' | 'normal' | 'dense' -> margin?: false | 'normal' | 'dense'.
  • Rename theme.palette -> theme.colors
    This should be less confusing when using the theme.
    I have seen any use case for an options argument, nor I can envision one now, two years later.
  • ButtonBase rename focusRipple -> disableFocusRipple for consistency with our API.
  • Hidden v2
    Will we still need this component with the design system package? The JS version is interesting.
  • Tab, rename TabIndicatorProps -> IndicatorProps.
  • Reduce the large size fontSize.
  • Rename the property component -> as? I need to run a Twitter Poll for this one.
  • npm, rename the package folder /es -> /src. This should reduce people confusion.
  • Change the controlled logic to use isControlled !== undefined (motivation).

This includes a proposal to consider the master branch as 3.x and next as 4.0.

We can either default to next which means we would need to backport changes where necessary or stay at master which would require a port of each PR to next. Either way merges between next and master should not be squashed since each commit in master is already squashed. Further squashing reduces the usability of git.

Someone with admin rights to the repo should protect the next branch against force pushes.

cc @mui-org/core-contributors

@rosskevin

This comment has been minimized.

@oliviertassinari oliviertassinari added the umbrella For grouping multiple issues to provide a holistic view label Nov 21, 2018
@oliviertassinari

This comment has been minimized.

@KaRkY
Copy link
Contributor

KaRkY commented Nov 21, 2018

What I would suggest for v4 is unifying callbacks. Table pagination onPageChange(event, value) while onChangeRowsPerPage(event). I would suggest that all callbacks thar return a value are callback(event, value).

@eps1lon

This comment has been minimized.

@oliviertassinari oliviertassinari changed the title v4 Breaking changes Material-UI v4: breaking changes Nov 21, 2018
@oliviertassinari

This comment has been minimized.

@TrySound

This comment has been minimized.

@TrySound
Copy link
Contributor

Can Grid be eliminated in favour of system?

@fzaninotto
Copy link
Contributor

How are we supposed to 👍 / 👎 on particular items? Also, please prefix this discussion with [RFC] if you're really open for discussion.

@eps1lon
Copy link
Member Author

eps1lon commented Nov 22, 2018

@fzaninotto Either on the linked issues or open a separate one. A productive discussion about multiple items is really hard within a single issue. This is just a tracking issue.

@jedwards1211
Copy link
Contributor

jedwards1211 commented Jan 30, 2019

theme.palette -> theme.colors

Have any devs reported being confused by theme.palette? If this change would actually help enough devs I think it would be worth it, but it would require wide-ranging changes to a lot of people's code that uses theme.palette, so I think it's only worth changing if very many devs have expressed confusion about this. Making a codemod isn't as trivial as some of the other changes because of the different ways one can access theme.palette (destructuring or not, etc.)

@jedwards1211
Copy link
Contributor

Icon/SvgIcon change the fontSize value property default -> normal.

I think "normal" actually has less concrete meaning than "default"?

@jedwards1211

This comment has been minimized.

@eps1lon

This comment has been minimized.

@Falieson
Copy link

Falieson commented Apr 8, 2019

Something changed in the tables alpha.1 to alpha.7 , I now have to add height (not lineHeight) to all rows. Update - I looked at the migration guide PR and see that the table height change is noted there.

@oliviertassinari
Copy link
Member

@Falieson Thank you for the problem report. Were you overriding the styles prior to the change of behavior? Any idea what changed?

@Falieson
Copy link

Falieson commented Apr 8, 2019

@oliviertassinari My table is derived from: https://material-ui.com/demos/tables/#sorting-amp-selecting . I just componentized and hooked it . I might have a fixed height on my tables which is causing the spread?


On a new note - would it be a lot of effort to include an upgrade script that will look for all instances of blocks that contain the properties moved from /core/ to /styles/ and update the import?

import {
  CardContent,
  createStyles,
  Theme,
  Typography,
  withStyles,
  WithStyles,
} from '@material-ui/core'
import {
  CardContent,
  Theme,
  Typography,
} from '@material-ui/core'
import {
  createStyles,
  withStyles,
  WithStyles,
} from '@material-ui/styles'

I tried a simple search/replace which gave me a big chunk of the work, but also created some mistakes. In my 6 week old app, I've had to update 120+ files, and its taken me over 5 hours.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 8, 2019

My table is derived from: https://material-ui.com/demos/tables/#sorting-amp-selecting

@Falieson I have taken the demo, and replaced v3 for v4, no issues: https://codesandbox.io/s/j3vjoyp7oy. I don't know what's going on. We need a reproduction.

On a new note - would it be a lot of effort to include an upgrade script that will look for all instances of blocks that contain the properties moved from /core/ to /styles/ and update the import?

I'm sorry, that's not needed. What lead you to this path? We don't promote it.

@Falieson
Copy link

Falieson commented Apr 8, 2019

@oliviertassinari
Tables: When I have a second I'll make a repro, try to figure out what the issue is that I'm coming across.
Imports:
not sure what you mean by not promoted?
createStyles, withStyles, WithStyles were moved from the /core package to the /styles package. imports fail without updating it.
When I write my applications I have a module folder for each of the react libraries I majorly use. Inside my material-ui library I've created abstractions and "Simple React" permutations of all the ways I use Material-UI. I'm not leading the charge here, I've found most mature React teams in SF are building abstraction libraries around third party libs.
Here's my MUI folder:
image

@oliviertassinari
Copy link
Member

@Falieson It's definitely not a bad idea. My point is that it won't change much at the end of the day. All the component demos are using @material-ui/core/styles. If you can avoid the changes, keep your code as is :).

@Falieson
Copy link

Falieson commented Apr 8, 2019

@oliviertassinari I didn't realize the properties were removed from the toplevel /core/ export but still available at /core/styles. Even still I would have had to refactor and split the imports like below

import {
  CardContent,
  Theme,
  Typography,
} from '@material-ui/core'
import {
  createStyles,
  withStyles,
  WithStyles,
} from '@material-ui/core/styles'

@jedwards1211
Copy link
Contributor

jedwards1211 commented Apr 9, 2019

@Falieson FWIW if you need to rename a bunch of import paths, you can use jscodeshift-transport

@oliviertassinari oliviertassinari added the component: select This is the name of the generic UI component, not the React module! label Apr 11, 2019
@pachuka
Copy link
Contributor

pachuka commented Apr 19, 2019

@eps1lon - Something I ran into today when upgrading from 3.9.3 -> 4.0.0-alpha.8. If you are explicitly using JssProvider/Jss to work around bundle splitting/class name generator issues, you need to make sure you upgrade your jss(9.8.7 -> 10.0.0-alpha.16) and react-jss(8.6.1 -> 10.0.0-alpha.16) packages to next versions as well otherwise I kept getting root is undefined errors on classes properties for all components.

Might want to add this to the migration-v3 doc.

@oliviertassinari
Copy link
Member

@pachuka We do no longer depend on react-jss. You can remove this dependency. You are correct regarding jss v9 👍. We should document it. We might also want to throw an explicit error. Do you have a reproduction for this problem? Do you want to add a quick note about it in the upgrade guide :) ?

@pachuka
Copy link
Contributor

pachuka commented Apr 20, 2019

@oliviertassinari, haha, sure I can create a codesandbox reproduction + PR note tomorrow.

@TidyIQ
Copy link
Contributor

TidyIQ commented Apr 20, 2019

Is there an issue with ThemeProvider at this stage? It was working fine previously but now I'm having issues.

I've created a custom theme and wrapped my apps in a ThemeProvider yet all components and makeStyles(theme => { ... }) are still using the default theme values.

For example:

// theme.tsx
const theme = createMuiTheme({
  palette: {
    primary: cyan
  }
});
export default theme;
// _app.tsx
class MyApp extends App {
  componentDidMount() {
    // Remove the server-side injected CSS
    const jssStyles = document.querySelector("#jss-server-side");
    if (jssStyles && jssStyles.parentNode) {
      jssStyles.parentNode.removeChild(jssStyles);
    }
  }
  render() {
    const { Component, pageProps } = this.props;
    console.log(theme) // THIS RETURNS THE CORRECT NEW CUSTOM THEME VALUES
    return (
      <Container>
        <Provider>
          <ThemeProvider theme={theme}>
            <CssBaseline />
            <Component {...pageProps} />
          </ThemeProvider>
        </Provider>
      </Container>
    );
  }
}
export default MyApp;
// index.tsx
....
<Typography color="primary">
  Test
</Typography>
....

The result of this is the typography color is set to the default purple[500] instead of cyan[500] as specified in createMuiTheme.

The same issue occurs when using:

const useStyles= makeStyles(theme => {
  changeColor: {
    color: theme.palette.primary.main
  }
}

Is this a known issue?

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 20, 2019

@TidyIQ Interesting. It's not the first time I hear about the problem. But I have never been able to reproduce it. What dependency versions are you using?

@TidyIQ
Copy link
Contributor

TidyIQ commented Apr 20, 2019

@oliviertassinari

 "dependencies": {
    "@material-ui/core": "^4.0.0-alpha.8",
    "@material-ui/icons": "^4.0.0-alpha.8",
    "@material-ui/styles": "^4.0.0-alpha.8",
    "@zeit/next-typescript": "^1.1.1",
    "next": "^8.1.0",
    "react": "^16.8.6",
    "react-dom": "^16.8.6"
  },
  "devDependencies": {
    "@types/next": "^8.0.3",
    "@types/react": "^16.8.13",
    "@types/react-dom": "^16.8.4",
    "@types/styled-jsx": "^2.2.8",
    "typescript": "^3.4.3"
  }

@oliviertassinari
Copy link
Member

@TidyIQ 😨 do you have a github repository I could have a look at?

@TidyIQ
Copy link
Contributor

TidyIQ commented Apr 20, 2019

Sure. It's a private repo but I'll make it public for a couple of hours so you can take a look.

https://github.com/TidyIQ/tidyiq_website

edit: Also just FYI, I followed the example repo here to set it up: https://github.com/mui-org/material-ui/tree/next/examples/nextjs-next-with-typescript

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 20, 2019

@TidyIQ It works great with a fresh yarn or npm install (rm package-lock.json). However, It's broken with your package-lock.json. I will see if I can find why and even better, add an explicit warning. Thank you!

@TidyIQ
Copy link
Contributor

TidyIQ commented Apr 20, 2019

Yeah you're right. I deleted all my node_modules and package_lock.json then reinstalled and now it's working fine. Strange... Well at least it's working now! Thanks for your help.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 20, 2019

@TidyIQ You have "@material-ui/styles" installed twice in package-lock.json: #15264.

@oliviertassinari
Copy link
Member

Thank you, everybody, for the feedback! We are marching toward stable v4.
We will replicate the same approach for v5.

@ee0pdt
Copy link
Contributor

ee0pdt commented Apr 30, 2019

@oliviertassinari The change that @Falieson refers to above (createStyles no longer exposed on /core) is a breaking change, but it is not documented anywhere. Worse, the typescript defs still show it being available in /core so VSCode doesn't warn me, but then the compiler fails. Should I raise an issue?

@oliviertassinari
Copy link
Member

@ee0pdt See #15532.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: select This is the name of the generic UI component, not the React module! discussion umbrella For grouping multiple issues to provide a holistic view
Projects
None yet
Development

No branches or pull requests